[go_router] Make `replace` use `pop` and `push` to generate a new `pageKey` #2747

ValentinVignal · 2022-10-28T16:24:06Z

Refactoresreplace to use pop and push to a unique page key is used every time replace is used.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

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

ValentinVignal · 2022-10-31T15:47:41Z

The test repo_checks failed because of the analyzer in the example of the go_router package:

    error - example/lib/books/src/screens/book_details.dart:7:8 - Target of URI doesn't exist: 'package:url_launcher/link.dart'. Try creating the file referenced by the URI, or try using a URI for a file that does exist. - uri_does_not_exist
    error - example/lib/books/src/screens/book_details.dart:58:13 - The method 'Link' isn't defined for the type 'BookDetailsScreen'. Try correcting the name to the name of an existing method, or defining a method named 'Link'. - undefi[...]rt:11:37 - Avoid method calls or property accesses on a "dynamic" target. - avoid_dynamic_calls
     info - example/test/sub_routes_test.dart:7:8 - The imported package 'go_router_examples' isn't a dependency of the importing package. Try adding a dependency for 'go_router_examples' in the 'pubspec.yaml' file. - depend_on_referenced_packages
     info - example/test/sub_routes_test.dart:11:37 - Avoid method calls or property accesses on a "dynamic" target. - avoid_dynamic_calls

I don't think that is related to my changes? Do you want me to try to fix it in this PR or should I do another PR instead?

chunhtai · 2022-10-31T15:48:53Z

no, it should be fixed by #2751 (comment)

Can you rebase off the latest main?

chunhtai · 2022-11-02T17:38:43Z

My current plan is to make the replace to push pageless route, in that case the pagekey would no longer matter. I think this pr will be obsolete see this conversation flutter/flutter#99112. I suggest we don't move forward with this pr. WDYT

ValentinVignal · 2022-11-03T02:24:46Z

About flutter/flutter#99112 I'm not in favor of using pageless pages from Navigator API directly. I will comment on the issue to explain why and my use cases.

However, our app is currently facing this bug, so I would say we could still push the fix. If we don't, the bug will still remain, which is an issue in my opinion.

chunhtai

Yes, you are right, it shouldn't stop this pr from merging. I left some comments

chunhtai · 2022-11-03T16:48:53Z

packages/go_router/lib/src/matching.dart


  /// Removes the last match.
-  void pop() {
+  void pop({bool asserts = true}) {


seems weird as an API, I think we can move the _debugAssertNotEmpty check to RouterDelegate.pop instead

and it should probably be wrapped in an assert

seems weird as an API,

Yes, I do agree. But I saw somewhere that someone was thinking of making some router's field private (there was an issue where this was discussed). So I didn't know whether or not pop would be not-accessible publicly at some point. In this case, it would have been fine.

But if not, yes, that's not a good idea to add this parameter.

I changed it in a50d124

…ith-push-count # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

chunhtai

LGTM, %nits

chunhtai · 2022-11-04T15:38:46Z

packages/go_router/lib/src/delegate.dart

    // Loop through navigators in reverse and call canPop()
    final int matchCount = _matchList.matches.length;
    for (int i = matchCount - 1; i >= 0; i -= 1) {
      final RouteMatch match = _matchList.matches[i];


you can also wrap this in an assert so that release mode will not even waste time calling into an empty function

Is 8e8d187 what you mean?

chunhtai · 2022-11-04T15:39:49Z

packages/go_router/pubspec.yaml

 description: A declarative router for Flutter based on Navigation 2 supporting
  deep linking, data-driven routes and more
-version: 5.1.2
+version: 5.1.4


With the newly merged PRs, I'll make it 5.1.6

…ith-push-count # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

ValentinVignal · 2022-11-13T07:23:54Z

@johnpryan is there anything you want me to do on this PR ?

packages/go_router/lib/src/delegate.dart

auto-submit · 2022-11-14T17:55:57Z

auto label is removed for flutter/packages, pr: 2747, due to - The status or check suite web_benchmarks_test CHROMIUM_BUILD:950363 has failed. Please fix the issues identified (or deflake) before re-applying this label.

The status or check suite web-platform_tests CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite linux-custom_package_tests CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite linux-build_all_packages CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite readme_excerpts has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite web-build_all_packages CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite downgraded_analyze has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite analyze CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite linux-platform_tests CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite android-build_all_packages CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite linux-build_all_packages CHANNEL:stable has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite web_benchmarks_test CHROMIUM_BUILD:768968 has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite android-build_all_packages CHANNEL:stable has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite android-platform_tests CHANNEL:master PACKAGE_SHARDING:--shardIndex 0 --shardCount 2 has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite analyze CHANNEL:stable has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite android-platform_tests CHANNEL:master PACKAGE_SHARDING:--shardIndex 1 --shardCount 2 has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite linux-custom_package_tests CHANNEL:stable has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite legacy_version_analyze CHANNEL:2.10.5 DART_VERSION:2.16.2 has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite web-platform_tests CHANNEL:stable has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite dart_unit_tests CHANNEL:stable has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite web-build_all_packages CHANNEL:stable has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite dart_unit_tests CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite legacy_version_analyze CHANNEL:3.0.5 DART_VERSION:2.17.6 has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

chunhtai · 2022-11-15T19:44:11Z

Not sure why the ci stuck @ValentinVignal can you rebase off the latest main branch and try again?

auto-submit · 2022-11-15T22:14:48Z

auto label is removed for flutter/packages, pr: 2747, due to - The status or check suite macos-platform_tests CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.

The status or check suite macos-custom_package_tests CHANNEL:stable has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite macos-custom_package_tests CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite ios-build_all_packages CHANNEL:stable has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite ios-build_all_packages CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.

…ith-push-count

…ith-push-count # Conflicts: # packages/go_router/CHANGELOG.md

ValentinVignal · 2022-11-16T07:42:09Z

@chunhtai It looks like it is fine now

[go_router] Make `replace` use `pop` and `push` to generate a new `pageKey` (#2747) * 🐛 Use pop and push in replace to generate a new pageKey * ✅ Test that replace creates a new page key * ⬆️ Increase the version number * ♻️ Move the asserts to the router deleguate * Wrap _debugAssertMatchListNotEmpty in an assert * Update packages/go_router/lib/src/delegate.dart Co-authored-by: John Ryan <[email protected]> Fix broken links in documentation update changelog

ValentinVignal requested a review from chunhtai as a code owner October 28, 2022 16:24

github-actions bot added the p: go_router label Oct 28, 2022

ValentinVignal added 3 commits November 1, 2022 09:04

🐛 Use pop and push in replace to generate a new pageKey

4f0c5ee

✅ Test that replace creates a new page key

9053078

⬆️ Increase the version number

9b20b5c

ValentinVignal force-pushed the go-router/replace-with-push-count branch from 3574eae to 9b20b5c Compare November 1, 2022 01:04

chunhtai reviewed Nov 3, 2022

View reviewed changes

ValentinVignal added 2 commits November 4, 2022 09:23

Merge remote-tracking branch 'upstream/main' into go-router/replace-w…

934f28e

…ith-push-count # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

♻️ Move the asserts to the router deleguate

a50d124

ValentinVignal requested a review from chunhtai November 4, 2022 01:34

chunhtai approved these changes Nov 4, 2022

View reviewed changes

chunhtai requested a review from johnpryan November 4, 2022 15:40

ValentinVignal added 2 commits November 5, 2022 22:03

Merge remote-tracking branch 'upstream/main' into go-router/replace-w…

5a039d9

…ith-push-count # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

Wrap _debugAssertMatchListNotEmpty in an assert

8e8d187

johnpryan approved these changes Nov 14, 2022

View reviewed changes

packages/go_router/lib/src/delegate.dart Outdated Show resolved Hide resolved

Update packages/go_router/lib/src/delegate.dart

0568cef

johnpryan added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 14, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 14, 2022

Merge branch 'main' into go-router/replace-with-push-count

30ac6cc

johnpryan added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 14, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 15, 2022

Merge remote-tracking branch 'upstream/main' into go-router/replace-w…

1ab67da

…ith-push-count

Merge remote-tracking branch 'upstream/main' into go-router/replace-w…

053d4fb

…ith-push-count # Conflicts: # packages/go_router/CHANGELOG.md

chunhtai merged commit d1f5f87 into flutter:main Nov 16, 2022

johnpryan mentioned this pull request Nov 17, 2022

[go_router]Fixes GoRouterState.location and GoRouterState.param to return correct value #2786

Merged

11 tasks

ValentinVignal mentioned this pull request Nov 23, 2022

[go_router] replace should reuse the same key as the popped page when possible flutter/flutter#115902

Closed

[go_router] Make replace use pop and push to generate a new pageKey #2747

[go_router] Make replace use pop and push to generate a new pageKey #2747

Uh oh!

Conversation

ValentinVignal commented Oct 28, 2022

Pre-launch Checklist

Uh oh!

ValentinVignal commented Oct 31, 2022

Uh oh!

chunhtai commented Oct 31, 2022

Uh oh!

chunhtai commented Nov 2, 2022

Uh oh!

ValentinVignal commented Nov 3, 2022

Uh oh!

chunhtai left a comment

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 left a comment

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!

ValentinVignal commented Nov 13, 2022

Uh oh!

Uh oh!

auto-submit bot commented Nov 14, 2022

Uh oh!

chunhtai commented Nov 15, 2022

Uh oh!

auto-submit bot commented Nov 15, 2022

Uh oh!

ValentinVignal commented Nov 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[go_router] Make `replace` use `pop` and `push` to generate a new `pageKey` #2747

[go_router] Make `replace` use `pop` and `push` to generate a new `pageKey` #2747