Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] Add path-based browser location handling #17829

Closed
wants to merge 1 commit into from

Conversation

slightfoot
Copy link
Member

@slightfoot slightfoot commented Apr 20, 2020

This change adds another flutter.web.usePathStrategy dart define flag so that you can build Flutter Web with a different location strategy. This change needs to land first before flutter/flutter#55232, so that the flutter tool can change it's local web-server behaviour so that the browser is given index.html for previously unknown URLs.

Example usage:
flutter build web --dart-define=flutter.web.usePathStrategy=true

Closes: flutter/flutter#33245 - Flutter_web navigation should provide a way to remove hash symbol(#)

/cc @jonahwilliams @yjbanov

@goderbauer
Copy link
Member

@chunhtai @mdebbar

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@passsy
Copy link

passsy commented Apr 20, 2020

@googlebot I fixed it.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@jonahwilliams
Copy link
Member

Can we name it something that isn't SCREAMING_CAPS? Perhaps flutter.web.usePath

@jonahwilliams
Copy link
Member

Or better yet flutter.web.usePathStrategy

@slightfoot
Copy link
Member Author

@jonahwilliams I am matching the previous behaviour already existing in flutter engine for this:

/// EXPERIMENTAL: Enable the Skia-based rendering backend.

@jonahwilliams
Copy link
Member

That should also be updated to something friendly like flutter.web.useSkiaBackend. No reason to add more technical debt if you don't need to

@slightfoot slightfoot changed the title [web] Add experimental path-based browser location handling [web] Add path-based browser location handling Apr 20, 2020
@slightfoot slightfoot force-pushed the path_location_strategy branch from eb24e62 to 66db24b Compare April 20, 2020 23:07
@slightfoot slightfoot force-pushed the path_location_strategy branch from 66db24b to cc80b21 Compare April 20, 2020 23:07
@slightfoot
Copy link
Member Author

@jonahwilliams updated to flutter.web.usePathStrategy

@mdebbar
Copy link
Contributor

mdebbar commented Apr 21, 2020

This is great! Thanks for working on this pain point facing many of flutter web users.

The implementation of the location strategy looks good to me. My concern is around other aspects of this change. Like asset loading and service worker.

If I understand correctly, assets and service worker caching, both use relative paths. This could cause issues when the user reloads the page. I could be completely wrong, so I would like @jonahwilliams to comment about this.

@jonahwilliams
Copy link
Member

Okay, thanks for the links @mdebbar I think I understand the situation better. It sounds like the solution is that users must specify an asset base directory to the framework/build and then flutter (framework side and service worker) must make http requests with these absolute URLs rather than the current strategy of using relative URLs?

@raul782
Copy link

raul782 commented Apr 24, 2020

@jonahwilliams yes that's what I had to do when testing both PRs
https://github.com/raul782/chasqui_frontend/blob/master/web/index.html#L12-L32

@mdebbar
Copy link
Contributor

mdebbar commented Apr 24, 2020

It sounds like the solution is that users must specify an asset base directory to the framework/build and then flutter (framework side and service worker) must make http requests with these absolute URLs rather than the current strategy of using relative URLs?

@jonahwilliams yep, that sounds right.

@mdebbar
Copy link
Contributor

mdebbar commented Apr 24, 2020

It looks like adding a <meta name="assetBase" content="/base/path/"> to index.html (like @raul782 did) should do the trick for assets. We read the meta tag here.

I'm not sure if this is enough for service worker or not.

@raul782
Copy link

raul782 commented Apr 26, 2020

@mdebbar, you mean adding a / to make it work relative the root path, this is what I did on the demo app above, https://github.com/raul782/chasqui_frontend/blob/master/web/index.html#L28

@jhancock4d
Copy link

Please make sure that arguments are also encoded and decoded in the query part of the URL. This is vital for deep linking and differentiation for SEO.

@jumper423
Copy link

@passsy @goderbauer @jonahwilliams
I and many people will be very grateful to you if you are approved.

#flutter/flutter#33245

@cbracken
Copy link
Member

cbracken commented Sep 9, 2020

@mdebbar can you take a look at this and decide what's the best way to proceed here? Looks like it's waiting on feedback.

I notice that flutter/flutter#55232 has been closed due to inactivity, but I don't have enough context to know whether that's due to this PR not having landed yet. If we do think we should proceed with this then we should probably revive that one.

@cbracken cbracken requested a review from mdebbar September 9, 2020 23:30
///
/// In order to use this [LocationStrategy] for an app, it needs to be set in
/// [engine.window.locationStrategy.locationStrategy]:
class PathLocationStrategy extends LocationStrategy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have a lot of duplicated code from the hashlocationstrategy. We should probably refactor out a mixin

@mdebbar
Copy link
Contributor

mdebbar commented Sep 9, 2020

This PR is the engine companion for flutter/flutter#55232, and should be closed for inactivity too.

The problem with the approach of this PR is that app developers can't implement their own location strategies, they can only choose from the ones we provide.

To solve that, we are going with a slightly different approach using JS interop. The PRs are still in draft mode because they were blocked by a JS interop issue for a while. The blocker has recently been fixed, so I'm planning to pick the PRs again and get them to the end line.

@mdebbar mdebbar closed this Sep 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flutter_web navigation should provide a way to customize url strategy