Switch to using Path URL Strategy #3585

DanTup · 2022-01-24T17:45:43Z

This changes the app to use the PageUrlStrategy in Flutter, which means URLs will just be standard like /foo instead of being added to the fragment like /#/foo.

@kenzieschmoll this seems to work well from my testing, including from VS Code (with a small tweak I'll ship in the next week or so), although I'm not certain there aren't pages with unusual navigation I may have missed.

Some things I'm not certain but should be checked before merging though"

It doesn't affect IntelliJ/Android Studio negatively (VS Code requires a change because it's over-encoding or URLs was causing issues here)
It doesn't affect anyone not using defaultHandler in the server... I've had to extend defaultHandler to be able to serve up index.html when a request comes in for something like /memory, because although navigations are usually done client-side, it's possible that the user hits Refresh or we try to launch directly on to a page which will now send /page_id to the server instead of just / which won't otherwise get the contents of index.html to launch the Flutter app.
Survey parsing may need updating

I'm not sure who best knows the answer to these questions though.

Fixes #2475.

DanTup · 2022-01-24T17:47:21Z

Oh also, survey parsing seems like it'll need updating - I'm not certain how to test that though. If you let me know I can ensure that's working correctly.

DanTup · 2022-01-24T19:27:52Z

@helin24 I don't know if this will affect IntelliJ/Android Studio. The reason VS Code needed a change was that the URLs were over-encoded which only seemed ok when we were using the fragment.

kenzieschmoll · 2022-01-25T00:45:59Z

Generating the survey url does need updating. The value of fromPage here should reflect which page a user clicked the survey link from (e.g. '', 'performance', 'inspector', etc.).

You can force the survey prompt to come up by deleting the .devtools file (rm ~/.flutter-devtools/.devtools) and manually changing the date on your computer to a date between 2/19 and 3/19. You'll still need to build the release version of the app to see this since the survey logic requires the server.

DanTup · 2022-01-25T10:14:48Z

Done! I extracted the code that extracts the page to a non-web-specific file so it was easier to add tests for it - I don't know if analytics_common was the right place for it, let me know if it's better elsewhere.

Btw when I was testing this it worked fine running from the CLI on a new port, but when launching on the normal port from VS Code, the app seemed heavily cached and I had to use Chrome devtools to "clear application data" before the change took affect. I don't know if this is just because it's not a real release or if other users may see this.

packages/devtools_app/lib/src/analytics/analytics_common.dart

kenzieschmoll · 2022-01-25T17:33:34Z

I found an issue when connecting to an app from the connect screen. When doing this, the 'inspector' page does not show up in the url. This is not specific to the inspector, but rather the first page shown after connecting DevTools to the running app.

Repro steps:

run devtools
copy and paste a vm service uri for a running flutter app into the connect screen
observe that 'inspector' is not present in the url. If you navigate to another page, and then click the inspector tab to go to the inspector page, you'll notice 'inspector' is now present.

DanTup · 2022-01-25T17:44:52Z

hmm, yeah - I can repro this. You can also just go direct to a URL like this:

http://127.0.0.1:9104/?uri=ws%3A%2F%2F127.0.0.1%3A59526%2FIRmSRy-94Gk%3D%2Fws

I think this is because we're just showing the first item in the tabbed control when there's no page, so it's valid to not have one and we'll show whatever is the first item.

If we never want this, I think probably we could check if there's a page in the URL after we connect, and if not, explicitly push the first visible screen?

kenzieschmoll · 2022-01-25T17:47:15Z

I think probably we could check if there's a page in the URL after we connect, and if not, explicitly push the first visible screen

Yeah, once we show a tab, we should verify that the page name is in the url, and if not, we should add it.

packages/devtools_server/lib/src/external_handlers.dart

kenzieschmoll · 2022-01-25T23:06:02Z

packages/devtools_server/lib/src/external_handlers.dart

@@ -97,7 +100,18 @@ Future<shelf.Handler> defaultHandler(
    if (debugMode) {
      return debugProxyHandler!(request);
    } else {
-      return buildDirHandler!(request);
+      // If the user hits Refresh when on a page being handled by the Flutter


The changes to this file will need to be applied to the g3 handler as well, but I have verified that things work as expected when that is done. I'll update the g3 handler when this change rolls into g3.

kenzieschmoll · 2022-01-25T23:06:40Z

packages/devtools_app/lib/main.dart


 void main() async {
+  usePathUrlStrategy();


this will need to be applied to the google3 main file. I'll take an action item to do this when this change is rolled into g3.

DanTup · 2022-01-26T14:43:13Z

I've pushed changes that should fix the URL thing. One updates the URL when the tab controller is initialised (so we know which first tab is being rendered if there isn't one in the QS), and one removes some code that had a bug if you went to /?page=memory then tried to navigate to another page, it was a bit confused about the current page. (Ultimately support for page= should probably be dropped, but IDEs are currently still using it).

packages/devtools_server/lib/src/external_handlers.dart

kenzieschmoll · 2022-01-26T16:57:08Z

Ultimately support for page= should probably be dropped, but IDEs are currently still using it

@DanTup @helin24 can we transition IDEs off of this pattern?

kenzieschmoll

lgtm once tests pass

helin24 · 2022-01-26T18:33:33Z

Ultimately support for page= should probably be dropped, but IDEs are currently still using it

@DanTup @helin24 can we transition IDEs off of this pattern?

This would mean instead of ?page=inspector /inspector?query=etc? Should be pretty easy for IntelliJ

DanTup · 2022-01-26T18:39:29Z

can we transition IDEs off of this pattern?

This would mean instead of ?page=inspector /inspector?query=etc? Should be pretty easy for IntelliJ

Yep, though we'll need to gate it on a version of DevTools that includes this PR (without the serveStaticFile change in this PR, the users would see 404s).

DanTup · 2022-01-27T11:19:26Z

@kenzieschmoll bots are passing, but only because I added a Future.microtask(() { around the navigation. It seems not valid to navigate during initState (or didChangeDependencies) - which kinda makes sense, it's odd that we're "navigating" (even if we are doing Router.neglect) as we're building (we can't do it sooner because we don't know which tab will be the "default" one).

Do you know if there's a better way to do this?

kenzieschmoll · 2022-01-27T20:27:30Z

packages/devtools_app/lib/src/scaffold.dart

+      Future.microtask(() {
+        final routerDelegate = DevToolsRouterDelegate.of(context);
+        Router.neglect(context, () {
+          routerDelegate.navigateIfNotCurrent(


Instead of using Future.microtask(), what about WidgetsBinding.instance.addPostFrameCallback ?

another idea - is there a better way to update the url that doesn't trigger a build in the first place?

another idea - is there a better way to update the url that doesn't trigger a build in the first place?

I haven't been able to find one. I thought what I was doing here with Router.neglect would do it, but even if it's not rebuilding, it does still throw being called here.

Instead of using Future.microtask(), what about WidgetsBinding.instance.addPostFrameCallback?

That seems to work (both in the browser, and the tests) so I've pushed that. I'm not sure it's an ideal fix, but I don't have any other ideas right now.

I think it is forcing build to be called because routerDelegate.navigateIfNotCurrent eventually calls notifyListeners. One idea is to hide the notifyListeners call behind a bool if (shouldNotify). shouldNotify could be an optional named parameter that defaults to true. I haven't tried this locally, so we'll need to make sure that this doesn't break any routing behavior if notifyListeners was false. This may cause more trouble than it is worth, but could be worth looking into.

My understanding is that notifyListeners() is how we notify Flutter that the URL may have changed (and in turn, it may update the visible URL to the user). If we remove that, I think the visible URL in the browser won't change at all (since we've only updated our own local state variables).

(If this doesn't seem right, I can do some testing of it though)

That makes sense. The postFrameCallback seems fine then.

kenzieschmoll · 2022-01-31T17:23:06Z

packages/devtools_app/web/index.html

@@ -10,6 +10,7 @@
 <head>
    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
+    <base href="/">


@elliette wanted to get your input on changes to this file to verify that this shouldn't have any effects on serving DevTools in g3 or with DDR / the Dart DevTools extension.

This looks safe to me! None of the serving logic should care about the base URL.

kenzieschmoll

Thanks for these changes, Danny. Can you please mirror the devtools_server/ changes in this PR to the new home of devtools_server inside dds?

external_handlers.dart --> https://github.com/dart-lang/sdk/blob/main/pkg/dds/lib/src/devtools/handler.dart#L26

server.dart --> https://github.com/dart-lang/sdk/blob/d1baf327a18dfc92476049a09985b645c08072f5/pkg/dds/lib/devtools_server.dart#L278

Please also leave the changes here for now, as we haven't completely migrated all code off of package:devtools_server and over to the dds implementation.

DanTup · 2022-01-31T17:49:52Z

Thanks! I want to re-test with these final tweaks in VS Code, and @helin24 may want to do the same for IntelliJ. If both are good I'll open the same in DDS.

DanTup · 2022-01-31T19:32:54Z

This still works in VS Code. I'll make a start on the DDS PR tomorrow. @helin24 I don't anticipate the changes since you last tested (which I believe is just wrapping the router call in addPostFrameCallback) but if you want to test it before we merge, let me know.

helin24 · 2022-01-31T19:36:12Z

@DanTup we don't need to test again; thanks for checking

DanTup · 2022-02-01T10:20:32Z

@kenzieschmoll

Thanks for these changes, Danny. Can you please mirror the devtools_server/ changes in this PR to the new home of devtools_server inside dds?

I see some code in here that seems to assume all paths will start with /devtools if notFoundHandler is not null. How is this currently used? I think in order to support this, the index.html would need to have <base href='/devtools' /> so that Flutters routing would update the URLs to the correct thing (/devtools/inspector).

If we don't expect the /devtools in the frontend paths, we'd need to know which urls (eg. out of /inspector and /foo) should serve up the static index.html and which should fall through to NotFoundHandler (which I think means having knowing a list of valid page URLs here).

kenzieschmoll · 2022-02-01T16:52:39Z

CC @bkonyi ☝️

…age set

DanTup · 2022-03-28T17:22:29Z

@kenzieschmoll

I've rebased this, and pushed DDS changes here:

https://dart-review.googlesource.com/c/sdk/+/239082/

This PR must not roll into the SDK without the changes above first landing, or DevTools will break. I do not know how g3 uses this and cannot test, but I've tested this by:

running ./tools/build.py --no-goma --mode release --arch x64 create_platform_sdk and testing dart devtools and `dart --observe foo.dart - this serves the existing (HashUrlStrategy) DevTools using the new handlers, and everything appears to work the same
Copying a built output of DevTools with this change into xcodebuild/ReleaseX64/dart-sdk/bin/resources/devtools and then testing both again - this serves the new (PathUrlStategry) DevTools and also appears to work, including navigating to the inspector and then hard-refreshing, which sends a request to the server for /inspector (or /authToken/devtools/inspector).

I concluded that serving always at /devtools and dropping the auth token would add some additional complexity (changing the URLs that IDEs are embedding, having to redirect from /#/inspector -> /devtools/inspector) and rewriting the base href would be simpler (although I don't think there's much in it so happy to change or discuss in more detail if you think we should do the other).

DanTup · 2022-04-05T10:40:48Z

@kenzieschmoll @bkonyi I've not been able to track down why this change fails with old-style (fragment) URLs. It seems that at some point during initialisation (after switching to UrlPathStrategy, although exactly when seemed inconsistent even if I added lots of delays) the fragment was being dropped.

As a fix, I decided that the best thing would be to detect legacy URLs right at the start, and redirect (with location.replace) to the new-style equivalent. This also has the benefit that no part of the DevTools code will need to handle legacy URLs anymore, it's all handled just be one small mapping before initialisation.

I've pushed that change to this PR so they'll need reviewing.

Since it's been a while since this was started, a summary:

DDS Change

CL: https://dart-review.googlesource.com/c/sdk/+/239082/

Changes the server to rewrite the base href if present in the page so support the UrlPathStrategy version of DevTools
Serves up the contents of index.html for requests to URLs like /inspector so linking directly to those pages (or hitting refresh) will not result in a 404 from the server

I believe these changes are safe to land before the DevTools changes (the opposite is not true).

DevTools Change

This PR

Changes the app to use PathUrlStrategy to remove the # fragments in the URLs
Adds a base href to support the above
Redirects old legacy URLs to new URLs before initialising the app

These changes require the serve has the changes above to function correctly.

Testing

To test, I did:

build release version of DevTools
build SDK (./tools/build.py --no-goma --mode release --arch x64 create_platform_sdk)
Overwrite DevTools in third_party/devtools/web and sdk/xcodebuild/ReleaseX64/dart-sdk/bin/resources/devtools with the newly built DevTools

Things I've tested:

running xcodebuild/ReleaseX64/dart-sdk/bin/dart devtools serves a version using Url paths
running xcodebuild/ReleaseX64/dart-sdk/bin/dart --observe=0 /tmp/foo.dart prints a valid URL that loads DevTools (it redirects to remove the fragment, and then functions as expected)
running VS Code using xcodebuild/ReleaseX64/dart-sdk as the active SDK spawns a working embedded devtools
running VS Code using xcodebuild/ReleaseX64/dart-sdk as the active SDK spawns working external devtools pages, and can navigate between pages using the server API (by running commands in VS Code to launch those other pages)

Things I'm not able to test:

IntelliJ/Android Studio
Google3

Future Changes

After both changes are landed, further changes to be made:

Unskip the two DDS tests that are currently skipped because they test the served page has a base href
Remove the # from the URL printed by dart --observe as it's no longer necessary (although the redirect handles it so it doesn't cause any problems)

DanTup · 2022-04-05T10:45:21Z

   info - test/url_utils_test.dart:36:69 - Missing a required trailing comma. - require_trailing_commas
   info - test/url_utils_test.dart:44:75 - Missing a required trailing comma. - require_trailing_commas

Pushed a fix for these, though for some reason I don't see these lints locally (nor can I see where require_trailing_commas is being enabled from), so if anyone has any ideas about that, I'm interested to know :-)

kenzieschmoll · 2022-04-05T19:37:26Z

for some reason I don't see these lints locally

This lint was enabled in a recent PR. If you merge master you should see the lint warnings (you may have to restart the analysis server or your IDE if the change is not picked up automatically).

DanTup · 2022-04-06T11:07:12Z

This lint was enabled in a recent PR. If you merge master you should see the lint warnings (you may have to restart the analysis server or your IDE if the change is not picked up automatically).

Ah, that's what it was. For some reason I thought GH Actions always ran the PR as it was on the source branch, but I can see from the logs that it merges master in first (which is better, I just didn't realise it did). Thanks!

packages/devtools_app/lib/src/url_strategy/web.dart

kenzieschmoll · 2022-04-11T20:02:13Z

Tested changes in google3. Google3 does not have a context where devtools is served under /devtools, so everything works as expected with a base href of /

DanTup · 2022-04-26T11:27:30Z

@kenzieschmoll I merged master into this (there were some conflicts because url_utils was removed, but I'd added to it here, so this brings it back).

Do you need to do anything in g3 before landing this (the DDS change has landed)? I see #3585 (comment) but suspect that needs doing as it's rolled into g3?

kenzieschmoll · 2022-04-26T15:09:31Z

Do you need to do anything in g3 before landing this (the DDS change has landed)?

Nope, I have a g3 CL ready to go that applies your changes to the g3 handler. You are free to land this.

kenzieschmoll · 2022-04-26T15:12:02Z

packages/devtools_app/lib/src/primitives/url_utils.dart

+/// This is a bit of a hack, as we have file paths instead of the explicit
+/// "package:uris" we'd like to have. This will be problematic for a use case
+/// such as "packages/my_package/src/utils/packages/flutter/".
+String getSimplePackageUrl(String url) {


can you remove this method and associated tests? (essentially removing everything that was removed as part of https://github.com/flutter/devtools/pull/3932/files#diff-0fb55973e4befafb1d8f5ee26a619a6500f6524ef0dfcf2c252f62aa0644076f)

Oops, good catch. I had to abort/re-do the merge and apparently didn't re-remove it the second time 😞

DanTup · 2022-04-26T16:12:37Z

@kenzieschmoll I did another final test and this all looked good, so I've landed it.

Anyone running the build_e2e.dart script will need to ensure they're on latest SDK to get the new server version for everything to work correctly.

Please ping me (and/or revert) if you think this has caused any serious issue. If you let me know once this version of the frontend has landed in the SDK, I'll un-skip those tests that rely on the base href (and tests again using the landed version of everything). Thanks!

DanTup mentioned this pull request Jan 24, 2022

Ensure there's no querystring before the fragment when launching the browser from the server #3579

Closed

kenzieschmoll reviewed Jan 25, 2022

View reviewed changes

packages/devtools_app/lib/src/analytics/analytics_common.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Jan 25, 2022

View reviewed changes

packages/devtools_server/lib/src/external_handlers.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Jan 25, 2022

View reviewed changes

packages/devtools_server/lib/src/external_handlers.dart Outdated Show resolved Hide resolved

kenzieschmoll reviewed Jan 25, 2022

View reviewed changes

kenzieschmoll reviewed Jan 26, 2022

View reviewed changes

packages/devtools_server/lib/src/external_handlers.dart Outdated Show resolved Hide resolved

kenzieschmoll approved these changes Jan 26, 2022

View reviewed changes

DanTup mentioned this pull request Jan 26, 2022

Update DevTools URL format Dart-Code/Dart-Code#3800

Closed

kenzieschmoll reviewed Jan 27, 2022

View reviewed changes

DanTup force-pushed the url-path-strategy branch from 40c5924 to bbcabd0 Compare January 31, 2022 16:16

DanTup requested a review from kenzieschmoll January 31, 2022 16:19

kenzieschmoll reviewed Jan 31, 2022

View reviewed changes

DanTup added 8 commits March 28, 2022 15:10

More extractCurrentPageFromUrl + tests into url_utils

c83dcd9

Ensure page URL includes the current tab when there was no existing p…

43d31ab

…age set

Allow navigating from /?page=foo to /bar correctly

31e134c

Restore accidentally deleted line

d96e5b6

Fix navigating during initState to fix URL

a47458b

Change Future.microtask to WidgetsBinding.instance.addPostFrameCallback

182a06f

Fix analysis errors after rebase on latest

e958e73

Add a comment about base href being replaced

0aee389

DanTup force-pushed the url-path-strategy branch from bbcabd0 to 0aee389 Compare March 28, 2022 17:17

Handle legacy URLs by redirecting them to new style URLs

fcfd4a7

Fix require_trailing_comma lints

8f71d21

DanTup mentioned this pull request Apr 5, 2022

Analyzer: Flaky results in IDE dart-lang/sdk#47567

Closed

kenzieschmoll reviewed Apr 8, 2022

View reviewed changes

packages/devtools_app/lib/src/url_strategy/web.dart Outdated Show resolved Hide resolved

Move url_strategy to config_specific

facf6e2

kenzieschmoll approved these changes Apr 12, 2022

View reviewed changes

Merge branch 'master' into url-path-strategy

737e87b

kenzieschmoll reviewed Apr 26, 2022

View reviewed changes

Remove redundant code/tests

ea2b177

DanTup merged commit 7dd8b98 into flutter:master Apr 26, 2022

kenzieschmoll mentioned this pull request Sep 8, 2022

DevTools V 2.15.0 broken when not accessed at root #4449

Closed

DanTup mentioned this pull request Jul 17, 2024

Devtools doesn't work if served through VsCode remote proxy #8067

Closed

Switch to using Path URL Strategy #3585

Switch to using Path URL Strategy #3585

Conversation

DanTup commented Jan 24, 2022 • edited Loading

DanTup commented Jan 24, 2022

DanTup commented Jan 24, 2022

kenzieschmoll commented Jan 25, 2022

DanTup commented Jan 25, 2022

kenzieschmoll commented Jan 25, 2022

DanTup commented Jan 25, 2022

kenzieschmoll commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanTup commented Jan 26, 2022

kenzieschmoll commented Jan 26, 2022

kenzieschmoll left a comment

Choose a reason for hiding this comment

helin24 commented Jan 26, 2022

DanTup commented Jan 26, 2022

DanTup commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenzieschmoll left a comment

Choose a reason for hiding this comment

DanTup commented Jan 31, 2022

DanTup commented Jan 31, 2022

helin24 commented Jan 31, 2022

DanTup commented Feb 1, 2022

kenzieschmoll commented Feb 1, 2022

DanTup commented Mar 28, 2022

DanTup commented Apr 5, 2022

DDS Change

DevTools Change

Testing

Future Changes

DanTup commented Apr 5, 2022

kenzieschmoll commented Apr 5, 2022

DanTup commented Apr 6, 2022

kenzieschmoll commented Apr 11, 2022

DanTup commented Apr 26, 2022

kenzieschmoll commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanTup commented Apr 26, 2022

DanTup commented Jan 24, 2022 •

edited

Loading