Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flutter_tools] Add experimental path-based browser location handling for Flutter Web. #55232

Closed
wants to merge 1 commit into from

Conversation

slightfoot
Copy link
Member

@slightfoot slightfoot commented Apr 20, 2020

Description

This enables the flutter run to start a local web-server that hosts index.html for unknown pages and thus allowing the Flutter Web JavaScript to detect the correct URL when using the new path location strategy.

This patch depends on my other PR for the engine here: flutter/engine#17829

Example usage

flutter run -d chrome --dart-define=flutter.web.usePathStrategy=true
flutter run -d chrome --release --dart-define=flutter.web.usePathStrategy=true
flutter build web --dart-define=flutter.web.usePathStrategy=true

Related Issues

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

Tests

Tests added.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

/cc @jonahwilliams @yjbanov

@fluttergithubbot fluttergithubbot added the tool label Apr 20, 2020
@fluttergithubbot
Copy link
Contributor

@fluttergithubbot fluttergithubbot commented Apr 20, 2020

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@Hixie
Copy link
Member

@Hixie Hixie commented Apr 20, 2020

We can't land this without tests. If you can find someone (or some time) to write tests in the near future, that would be great; otherwise we should probably close the PR until you have the time to write tests.

@slightfoot
Copy link
Member Author

@slightfoot slightfoot commented Apr 20, 2020

@Hixie Added tests

@slightfoot slightfoot closed this Apr 20, 2020
@slightfoot slightfoot reopened this Apr 20, 2020
@slightfoot slightfoot force-pushed the path_location_strategy branch 2 times, most recently from a490bab to b8e306f Compare Apr 20, 2020
@@ -834,7 +862,7 @@ class ReleaseAssetServer {
'Content-Type': mimeType,
});
}
if (request.url.path == '') {
if (request.url.path == '' || enableIndexRewrite) {
Copy link
Member

@jonahwilliams jonahwilliams Apr 20, 2020

Choose a reason for hiding this comment

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

Is there any downside to always returning the index.html here?

Copy link
Member

@jonahwilliams jonahwilliams Apr 20, 2020

Choose a reason for hiding this comment

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

If this is the only change needed for all this plumbing, it seems like it would be easiest to always return the index, especially if this becomes the default in the future

Copy link
Member Author

@slightfoot slightfoot Apr 20, 2020

Choose a reason for hiding this comment

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

This is to make it optionally always return the index such that we don't break the previous behaviour.

Copy link
Member Author

@slightfoot slightfoot Apr 20, 2020

Choose a reason for hiding this comment

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

And it makes the release web server match the behaviour of the debug web server.

@@ -337,6 +352,12 @@ class WebAssetServer implements AssetReader {
file = globals.fs.file(webPath);
}

if (!file.existsSync() && enableIndexRewrite) {
Copy link
Member

@jonahwilliams jonahwilliams Apr 20, 2020

Choose a reason for hiding this comment

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

Oh I see, this one too

Copy link
Member Author

@slightfoot slightfoot Apr 20, 2020

Choose a reason for hiding this comment

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

This handles the case if index.html is missing we return a 404.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Can we tell the difference between a request to some/nav/path and asset.jpg? I would be okay with the former always returning index.html in either mode, but the later should always be a 404

@slightfoot slightfoot force-pushed the path_location_strategy branch 2 times, most recently from 4563670 to c643196 Compare Apr 20, 2020
@slightfoot
Copy link
Member Author

@slightfoot slightfoot commented Apr 20, 2020

Can we tell the difference between a request to some/nav/path and asset.jpg? I would be okay with the former always returning index.html in either mode, but the later should always be a 404

It does already prioritise the assets over the unknown paths. But no there is way to detect the different types of request. Because a route name can be anything including classed 'something.jpg' (if they wanted)

@jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Apr 20, 2020

Do you know how do other sorts of dev servers for web frameworks handle it? I'd rather not introduce drastically different behavior for these two modes.

Even if the developer has opted into the path strategy, it seems unfriendly to return the index if they incorrectly specify an asset.

@slightfoot
Copy link
Member Author

@slightfoot slightfoot commented Apr 20, 2020

Url rewrites on live servers works this way.. @passsy uses Firebase Hosting.
https://firebase.google.com/docs/hosting/full-config#rewrites

https://pascalwelsch.com/testing.jpg

@jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Apr 21, 2020

Seems like a reasonable option would be to support rewrites on URLs without a file extension?

@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Apr 21, 2020

JS tools like webpack solve this on the client side. Webpack has a config called publicPath (<-- this link has good documentation explaining how it works).

Basically, the idea is that the app developer provides a basePath where all assets should be loaded from. The client side takes the relative asset path requested by the app, then prepends basePath to it.

Example: Let's say we have an app that's being served on https://hosting-server.com/myapp/. That's the root path to the app. The developer would provide basePath="/myapp/". The user could be navigating to https://hosting-server.com/myapp/page1 but all asset requests should go to https://hosting-server.com/myapp/assets/image.png instead of https://hosting-server.com/myapp/page1/assets/image.png.

@jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Apr 21, 2020

For the "asset" assets, we essentially always use a baseUrl of flutter_assets. This would be needed for network images and friends then?

I think it would be easier to think about this change if there were some examples of apps that worked with hash-based navigation that would otherwise break when switching to this to new strategy

@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Apr 21, 2020

@jonahwilliams the main difference is that the hash path has no effect on request paths and the server doesn't receive the hash path.

index.html

When you reload this url: http://localhost:8080/#/page1 the server will get the following request: http://localhost:8080/. Even if you move to http://localhost:8080/#/page2 and reload, the server will still receive the same http://localhost:8080/.

But with the new location strategy, you would be navigating to http://localhost:8080/page1 and the server would receive the request: http://localhost:8080/page1. So the server needs to be able to handle this correctly.

assets

Using hash paths, when the user is on http://localhost:8080/#/page1 and tries to load assets/image.png, the request that would go to the server is http://localhost:8080/assets/image.png (notice how the hash path is omitted).

Using the new location paths, when the user is on http://localhost:8080/page1 and tries to load assets/image.png, the request that would go to the server is http://localhost:8080/page1/assets/image.png. So either:

  1. The server needs to somehow know that the actual asset path starts at /assets/image.png instead of /page1/assets/image.png.
  2. The client needs to send the request to http://localhost:8080/assets/image.png.

When I say assets, I mean anything that's loaded over the network (fonts, images, etc).

Also, I'm not sure how this will affect service worker caching since it uses a relative path to cache, right @jonahwilliams?

@RedBrogdon
Copy link
Contributor

@RedBrogdon RedBrogdon commented Apr 21, 2020

CC @johnpryan.

… for Flutter Web.

Added tests.

Updated define name to: flutter.web.usePathStrategy
@slightfoot slightfoot self-assigned this Apr 23, 2020
@jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Apr 30, 2020

I spoke offline with @mdebbar and @yjbanov about the best way to handle this in the tool. We'd like to keep the behavior consistent in both modes, so lets do the fallback regardless of whether the flag was provided.

Next, we want to ensure that we don't incorrectly fallback for assets that have typos. Since all assets should have assets in the URL, we could refuse to fallback to the index in that case

@jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Apr 30, 2020

For service worker changes, I can handle that as a follow up.

@zanderso zanderso added the waiting for customer response label May 7, 2020
@slightfoot
Copy link
Member Author

@slightfoot slightfoot commented May 13, 2020

@jonahwilliams what's happening with this?

@zanderso zanderso requested a review from jonahwilliams May 14, 2020
@zanderso zanderso removed the waiting for customer response label May 14, 2020
@jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented May 15, 2020

@slightfoot I asked for changes the comment #55232 (comment)

There are also failing tests that need to be addressed

@slightfoot
Copy link
Member Author

@slightfoot slightfoot commented May 15, 2020

@jonahwilliams I'd didn't realise that your statements were a request for change and they are a little unclear to me. I just want to restate to get clarification before edits.

  1. Remove the flag, so we redirect anyway since this won't effect the previous hash URL requests.
  2. Don't perform any redirect for /assets/ url's so they still surface 404s' for typos' in asset names.
  3. We should update the index.html template so that it uses '/' based absolute URIs not relative paths so that "sub-directory" requests get the linked content correctly.

Is that correct?

@jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented May 17, 2020

Yes, that SGTM

@zanderso zanderso added the waiting for customer response label May 21, 2020
@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Jun 2, 2020

@slightfoot have you had a chance to look into the changes?

@mdebbar mdebbar self-requested a review Jun 9, 2020
@jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Jun 25, 2020

Because of the lack of activity on this PR I'm going to close it to remove from our review queue. Please feel free to send us a new PR or discuss in an issue anytime.

@mehmetkose
Copy link

@mehmetkose mehmetkose commented Aug 10, 2020

lack of activity means you can close PR's and throw away? interesting.

@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Aug 15, 2020

@mehmetkose keeping the PR open with no activity increases the overhead for reviewers who go through the queue of open PRs.

I assure you nothing is thrown away. The branch that contains the changes is still there. The author (or anyone else) can easily clone this PR when they have more time available.

@anumontu
Copy link

@anumontu anumontu commented Sep 24, 2020

Any update on this ?

@jumper423
Copy link

@jumper423 jumper423 commented Sep 24, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool waiting for customer response
Projects
None yet
Development

Successfully merging this pull request may close these issues.