[go_router] Expose full `Uri` on `GoRouterState` in `GoRouterRedirect` #5742

ChopinDavid · 2023-12-22T20:14:43Z

A number of developers have voiced the desire to be able to know whether a GoRouterState is the result of a deep-link or in-app navigation from within their GoRouter's redirect method. This can be accomplished by exposing the Uri's scheme and host on the GoRouterState. This way, we can know whether the GoRouterState is the result of a deep-link by checking state.uri.scheme != null or state.uri.host != null.

This PR would close #103659.

No tests were broken as a result of this change, and we have added coverage for this change through new tests.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

google-cla · 2023-12-22T20:14:48Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

ChopinDavid · 2023-12-22T20:20:40Z

@SuperWes It seems Dan would need to sign the Google CLA to pass all checks as the first commit of this PR was authored by him. See here.

chunhtai

Since we are migrating to uri, can you double check every place with todo linked with flutter/flutter#124045 ?

chunhtai · 2023-12-27T21:57:51Z

packages/go_router/lib/src/parser.dart

@@ -86,7 +86,8 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
        // to uri, empty path check might be required here; see
        // https://github.com/flutter/packages/pull/5113#discussion_r1374861070


The entire todo and ignores above can be removed, and we need to do empty path check here as well per comment above

The entire todo and ignores above can be removed, and we need to do empty path check here as well per comment above

I am not sure that we necessarily need to do the empty path check. It seems that you feel we need to do this check so that we do not re-introduce Issue #133928. But when launching our company's app through a deep link with no path (https://www.wwt.com) from a "dead" state, we are not seeing this GoException: no routes for location error message and the app launches as expected.

Maybe you can check to see if this issue has been re-introduced on your side?

Or, was there maybe another reason why we felt we had to add the empty path check other than re-introducing Issue #133928?

Let me check and I will get back to you

I am testing in android, it looks like when cold start without the path the uri doesn't contain the host name and scheme. I am not sure what happen here.

If I warm start without the path, it would show no route for location. It seems to me we need to handle empty path here.

for code start, it is going through this code path

packages/packages/go_router/lib/src/router.dart

Line 538 in 36a7b99

// TODO(chunhtai): Clean up this once `RouteInformation.uri` is available

this need to be migrated as well

Yeah, I am actually seeing that we get the no routes for location error when launching www.domain.com while the app is already running in the background.

It's almost as if we need to treat all www.domain.com URIs as www.domain.com/ so that the location defaults to /. It makes sense that having an empty-string location wouldn't match anything.

For context, I am looking here, where no match is found for an empty string, even for the / path. It maybe makes sense to have an empty-string location redirect to the GoRouter's initialLocation.

Does this make sense, or is there a better approach? This still doesn't, for me, explain why we don't get the no routes for location error when launching www.domain.com while the app is not running in the background.

@chunhtai ~~Does this sound like the right approach to you?~~ Nevermind, we think we found the right approach, new commit imminent.

@chunhtai we went ahead and added the empty path check to ensure that routeInformation with an empty .uri.path has a / appended to the end of it. This behavior matches what we had previously been doing with routeInformation.location.

The issue is that there are a few tests (3) failing now. It seems like some of these tests could be rewritten to accommodate for the changes we've made, but we are hesitant to do this in fear of breaking functionality for others...

Let me know what you think is a good path forward for this.

ChopinDavid · 2023-12-28T05:08:00Z

@chunhtai I went ahead and migrated all occurrences of RouteInformation.location per the migration guide. I did have a few questions:

It seems that this TODO can go ahead and be removed?
I went ahead and removed the didPushRoute override entirely from InformationProvider. Does this sound correct?

chunhtai · 2023-12-28T17:21:33Z

packages/go_router/lib/src/information_provider.dart

@@ -109,7 +105,7 @@ class GoRouteInformationProvider extends RouteInformationProvider
    final bool replace;
    switch (type) {
      case RouteInformationReportingType.none:
-        if (_valueInEngine.location == routeInformation.location &&
+        if (_valueInEngine.uri.path == routeInformation.uri.path &&


This would also need to check the query parameter and hash fragment

So something like

if (_valueInEngine.uri.path == routeInformation.uri.path && _valueInEngine.uri.queryParameters == routeInformation.uri.queryParameters && _valueInEngine.uri.fragment == routeInformation.uri.fragment && const DeepCollectionEquality() .equals(_valueInEngine.state, routeInformation.state))

?

you probably need to do collection equality for query parameter

I believe I have done these correctly, I went ahead and added DeepCollectionEquality checks to the queryParameters and fragments, as well as the paths and states.

chunhtai · 2023-12-28T17:23:18Z

packages/go_router/lib/src/information_provider.dart

@@ -149,8 +142,8 @@ class GoRouteInformationProvider extends RouteInformationProvider

  void _setValue(String location, Object state) {
    final bool shouldNotify =
-        _value.location != location || _value.state != state;
-    _value = RouteInformation(location: location, state: state);
+        _value.uri.path != location || _value.state != state;


same here this should also account for query parameter and hashfragment

Same as above ⬆️, believe these have been fixed but let me know if you think otherwise.

chunhtai · 2023-12-28T17:25:38Z

packages/go_router/lib/src/parser.dart

@@ -86,7 +86,8 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
        // to uri, empty path check might be required here; see
        // https://github.com/flutter/packages/pull/5113#discussion_r1374861070


Let me check and I will get back to you

chunhtai · 2023-12-28T17:28:33Z

@chunhtai I went ahead and migrated all occurrences of RouteInformation.location per the migration guide. I did have a few questions:

It seems that this TODO can go ahead and be removed?

It seems so, yes can you try to remove it and see if the analyzer complains or not

I went ahead and removed the didPushRoute override entirely from InformationProvider. Does this sound correct?

That sounds correct.

chunhtai · 2023-12-28T22:37:51Z

looks like the CLA is not signed, this can happen if the commit history contain too many unrelated commit, you probably should do a sqush and force push, or just reset

ChopinDavid · 2023-12-29T03:44:13Z

looks like the CLA is not signed, this can happen if the commit history contain too many unrelated commit, you probably should do a sqush and force push, or just reset

The first commit on this branch was made by another member of my team. He probably just needs to sign the CLA. I will check with him in the coming days.

ChopinDavid · 2024-01-08T18:12:35Z

@chunhtai We believe we have this in a good state. We ensured all tests are passing (without modifying said tests) and added new tests to cover the changes that were made.

Please let me know if you are unsure about any of these changes. Would love to see this get merged in!

hannah-hyj · 2024-01-11T00:20:55Z

packages/go_router/lib/src/information_provider.dart

    final bool shouldNotify =
-        _value.location != location || _value.state != state;
-    _value = RouteInformation(location: location, state: state);
+        !deepCollectionEquality.equals(_value.uri.path, uri.path) ||


code here is duplicated to above, extract it to a function

Should be resolved via 5597ddb

Please, let me know if this does not look right to you.

hannah-hyj · 2024-01-11T00:25:11Z

packages/go_router/lib/src/information_provider.dart

@@ -274,11 +278,4 @@ class GoRouteInformationProvider extends RouteInformationProvider
    _platformReportsNewRouteInformation(routeInformation);
    return SynchronousFuture<bool>(true);
  }
-


why remove this?

See here.

And here ⬇️⬇️⬇️
didPushRoute is now deprecated, and didPushRouteInformation should be used instead.

bawahakim · 2024-02-08T01:31:40Z

FWIW, works well for me (at least on Android), and looking forward to having this in release, thanks @ChopinDavid ! 🚀

ZanderZhan · 2024-02-08T01:52:05Z

Works well for me too, it's good having this in release as soon as possible, thanks @ChopinDavid ! 🚀

hannah-hyj · 2024-02-08T01:54:21Z

@ChopinDavid can you resolve the conflicts for this PR so it can be merged ?

hannah-hyj · 2024-02-08T01:56:53Z

@johnpryan can you take a second look at this PR since chun-heng is on vacation?

ChopinDavid · 2024-02-08T14:48:06Z

Hi guys, I will resolve these conflicts a bit later today. Thanks for the help everyone! 🙂🚀

…rmation.uri - DJC/WE/DC

…pty - DC/JD

…e - DC/JD

…ange notifications - DC

…e presence of uri host/scheme in deep link - DC

…der _value has changed to a method - DC

ChopinDavid · 2024-02-08T19:57:25Z

Looks like everything should be good to go now

…Redirect` (flutter/packages#5742)

flutter/packages@11152d2...0a69259 2024-02-09 [email protected] [local_auth]: Renamed `local_auth_ios` to `local_auth_darwin` (flutter/packages#5809) 2024-02-09 [email protected] [webview_flutter] Add listener for content offset (flutter/packages#3444) 2024-02-09 [email protected] [go_router] Expose full `Uri` on `GoRouterState` in `GoRouterRedirect` (flutter/packages#5742) 2024-02-09 [email protected] [webview_flutter] Minor test cleanup (flutter/packages#6031) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

flutter#5742) A number of developers have voiced the desire to be able to know whether a `GoRouterState` is the result of a deep-link or in-app navigation from within their `GoRouter`'s `redirect` method. This can be accomplished by exposing the `Uri`'s `scheme` and `host` on the `GoRouterState`. This way, we can know whether the `GoRouterState` is the result of a deep-link by checking `state.uri.scheme != null` or `state.uri.host != null`. This PR would close [#103659](flutter/flutter#103659 (comment)). No tests were broken as a result of this change, and we have added coverage for this change through new tests.

ChopinDavid requested a review from chunhtai as a code owner December 22, 2023 20:14

github-actions bot added the p: go_router label Dec 22, 2023

ChopinDavid mentioned this pull request Dec 22, 2023

[go_router] Prevent access to route when navigated to via a deep link flutter/flutter#103659

Closed

chunhtai reviewed Dec 27, 2023

View reviewed changes

ChopinDavid force-pushed the Issue103659 branch from 0f4af14 to d7ed82d Compare December 28, 2023 04:38

ChopinDavid requested review from hellohuanlin and bparrishMines as code owners December 28, 2023 04:38

github-actions bot added platform-android platform-ios p: webview_flutter labels Dec 28, 2023

ChopinDavid mentioned this pull request Dec 28, 2023

[go_router][url_launcher] migrate deprecated RouteInformation.location usage flutter/flutter#124045

Closed

github-actions bot removed platform-android platform-ios p: webview_flutter labels Dec 28, 2023

chunhtai reviewed Dec 28, 2023

View reviewed changes

chunhtai requested a review from hannah-hyj December 28, 2023 22:38

bparrishMines removed their request for review January 3, 2024 23:09

ChopinDavid force-pushed the Issue103659 branch from 71e48b5 to b71811b Compare January 8, 2024 17:30

ChopinDavid force-pushed the Issue103659 branch 2 times, most recently from 029a273 to a84020e Compare January 8, 2024 21:26

hannah-hyj reviewed Jan 11, 2024

View reviewed changes

hellohuanlin removed their request for review January 16, 2024 19:35

ChopinDavid requested a review from hannah-hyj January 17, 2024 14:57

dancout and others added 15 commits February 8, 2024 12:59

97681 - Replacing deprecated routeInformation.location with routeInfo…

a3e1bc5

…rmation.uri - DJC/WE/DC

sc-97681 - Bumped min flutter version to >=3.12.0 - DC

54cbbdc

sc-97681 - Added tests - DC

16ded06

Update version number - DC

4aac028

Updated CHANGELOG.md - DC

0769bb3

Migrated all instances of - DC

96b6191

Removed unnecessary_overrides ignore and associated TODO - DC

a3738c4

Made it so that we append '/' to the end of our uri if its path is em…

6045d94

…pty - DC/JD

Made it so that links such as 'www.domain.com/?' find a matching rout…

1284808

…e - DC/JD

Added DeepCollectionEquality to conditions for InformationProvider ch…

3f2b327

…ange notifications - DC

Fixed broken tests - DC

541af50

Added some tests ensuring that correct routes are still matched in th…

515be47

…e presence of uri host/scheme in deep link - DC

Refactored duplicated logic for checking whether the InformationProvi…

edf550d

…der _value has changed to a method - DC

Upped min flutter version to 3.13.0 - DC

fd9c9c7

Upped min dart sdk to 3.1.0 - DC

8e8af62

ChopinDavid force-pushed the Issue103659 branch from b54cb24 to 8e8af62 Compare February 8, 2024 19:13

johnpryan approved these changes Feb 8, 2024

View reviewed changes

hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 9, 2024

auto-submit bot merged commit c269727 into flutter:main Feb 9, 2024

engine-flutter-autoroll mentioned this pull request Feb 12, 2024

Roll Packages from 11152d2bc8f3 to 0a692590a885 (4 revisions) flutter/flutter#143306

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 12, 2024

c269727 [go_router] Expose full Uri on GoRouterState in `GoRouter…

3c2c889

…Redirect` (flutter/packages#5742)

reidbaker mentioned this pull request Feb 19, 2024

[in_app_purchase_android] Add UserChoiceBilling mode. #6162

Merged

16 tasks

		@@ -86,7 +86,8 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
		// to uri, empty path check might be required here; see
		// https://github.com/flutter/packages/pull/5113#discussion_r1374861070

[go_router] Expose full Uri on GoRouterState in GoRouterRedirect #5742

[go_router] Expose full Uri on GoRouterState in GoRouterRedirect #5742

Uh oh!

Conversation

ChopinDavid commented Dec 22, 2023

Pre-launch Checklist

Uh oh!

google-cla bot commented Dec 22, 2023

Uh oh!

ChopinDavid commented Dec 22, 2023

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChopinDavid Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChopinDavid Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChopinDavid Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChopinDavid Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChopinDavid commented Dec 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Dec 28, 2023

Uh oh!

chunhtai commented Dec 28, 2023

Uh oh!

ChopinDavid commented Dec 29, 2023

Uh oh!

ChopinDavid commented Jan 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChopinDavid Jan 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bawahakim commented Feb 8, 2024

Uh oh!

ZanderZhan commented Feb 8, 2024

Uh oh!

hannah-hyj commented Feb 8, 2024

Uh oh!

hannah-hyj commented Feb 8, 2024

Uh oh!

[go_router] Expose full `Uri` on `GoRouterState` in `GoRouterRedirect` #5742

[go_router] Expose full `Uri` on `GoRouterState` in `GoRouterRedirect` #5742

ChopinDavid Dec 28, 2023 •

edited

Loading

ChopinDavid Dec 28, 2023 •

edited

Loading

ChopinDavid Jan 2, 2024 •

edited

Loading

ChopinDavid Jan 2, 2024 •

edited

Loading

ChopinDavid Jan 13, 2024 •

edited

Loading

ChopinDavid commented Feb 8, 2024 •

edited

Loading