[go_router] When `replace` is called, reuse the same key when possible

[ValentinVignal]

Should fix https://github.com/flutter/flutter/issues/115902

I've introduced PageKey to be used as a key for the pages instead of ValueKey<String>. In that way, it is easy to know if 2 keys are from the same path or not.

I'm not too confident about my changes in

• packages/go_router/lib/src/match.dart: It uses the hashCode
• packages/go_router/lib/src/builder.dart: This is the only place I had to use fullPath. So maybe we could use path directly?

Pre-launch Checklist

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• [x] I signed the CLA.
• [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• [x] I listed at least one issue that this PR fixes in the description above.
• [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• [x] I updated/added relevant documentation (doc comments with ///).
• [x] I added new tests to check the change I am making, or this PR is test-exempt.
• [x] All existing and new tests are passing.

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

[ValentinVignal]

@chunhtai I refactored push and replace to use 2 common private methods in ♻️ Make replace and push use common methods

[MarufHassan17]

Please add the option which displays depreciated with the new term in VSCode and Android Studio when hovering over replaceNamed

[ValentinVignal]

@chunhtai What do you think about this implementation? I didn't implement replaceNamed because I read you wanted to remove the named route API at some point (https://github.com/flutter/flutter/issues/107729)

[ValentinVignal]

It looks like the checks are failing because of go_router_builder I believe https://github.com/flutter/packages/pull/2977 should fix it

[chunhtai]

@chunhtai What do you think about this implementation? I didn't implement replaceNamed because I read you wanted to remove the named route API at some point (flutter/flutter#107729)

We decide to keep named location since we couldn't come up with a better alternative, and functionality wise it works fine.

[ValentinVignal]

@chunhtai What do you think about this implementation? I didn't implement replaceNamed because I read you wanted to remove the named route API at some point (flutter/flutter#107729)

We decide to keep named location since we couldn't come up with a better alternative, and functionality wise it works fine.

Ok I can add replaceNamed, but before I do that. Is the implementation fine with you?

[ValentinVignal]

I implemented replaceNamed in ✨ Add replaceNamed

[ValentinVignal]

Please add the option which displays depreciated with the new term in VSCode and Android Studio when hovering over replaceNamed

@MarufHassan17, I'm not sure I understand what you mean? Did you mean I should add the @Deprecated decorator to replaceNamed? If yes, replaceNamed has already been deleted in https://github.com/flutter/packages/pull/2848 and got renamed into pushReplacementNamed (it is a breaking change)

[ValentinVignal]

@chunhtai What do you think of this implementation?

Or do you think the key should always be reused when the user calls replace ? It's either we

• Only reuse the same key when the location is the same. But this prevents the user to be able from reusing/preserving any state between 2 similar routes (ex : /my-form/:form-id and /my-form/:form-id/edit) with 2 different locations. And it will force the user to reuse the same route (ex: /my-form/:form-id and /my-form/:form-id?edit=true). I don't know whether it is good or bad.
• Always reuse the same key. We let the user decide whether or not he wants to keep the state of its widget tree by using replace or lose it by using pushReplacement. This will allow to have animations/reuse the state between 2 similar routes with a different location (ex : /my-form/:form-id and /my-form/:form-id/edit) (and I don't know whether it is good or bad either). However, using the location of the routes for the keys wouldn't make so much sense anymore as the new route could have nothing related to the location of the reused key. And it will need to be changed.

What do you think?

[ValentinVignal]

@chunhtai When you review, could you take a look at https://github.com/flutter/packages/pull/2846#issuecomment-1384778791

Or do you think the key should always be reused when the user calls replace ? It's either we

• Only reuse the same key when the location is the same. But this prevents the user to be able from reusing/preserving any state between 2 similar routes (ex : /my-form/:form-id and /my-form/:form-id/edit) with 2 different locations. And it will force the user to reuse the same route (ex: /my-form/:form-id and /my-form/:form-id?edit=true). I don't know whether it is good or bad.
• Always reuse the same key. We let the user decide whether or not he wants to keep the state of its widget tree by using replace or lose it by using pushReplacement. This will allow to have animations/reuse the state between 2 similar routes with a different location (ex : /my-form/:form-id and /my-form/:form-id/edit) (and I don't know whether it is good or bad either). However, using the location of the routes for the keys wouldn't make so much sense anymore as the new route could have nothing related to the location of the reused key. And it will need to be changed.

What do you think?

I'm not sure which implementation would be best

[chunhtai]

@chunhtai When you review, could you take a look at [#2846 (comment)]

The later makes more sense, we should always reuse key when replace is called. Having hidden logic like checking location first makes the API harder to predict.

[ValentinVignal]

I think the pr looks good except the replace should always reuse the key.

@chunhtai I've changed the business logic to always reuse the same key

[ValentinVignal]

@chunhtai I updated the doc in docs: Update the doc of replace to mention animation and state

Tell me if it is good enough for you

[ValentinVignal]

@chunhtai Sorry for the misses, I added the doc in docs: Add more doc to replace and replaceNamed

[johnpryan]

@stuartmorgan @ditman can you help us figure out why CI is failing?

[ditman]

@stuartmorgan @ditman can you help us figure out why CI is failing?

@johnpryan "submit-queue" is the check that asserts that whatever is at the tip of flutter/packages main branch is passing all post-submit tests.

It looks red now, because flutter/packages main is undergoing testing (here), but once that finishes correctly, it'll turn green in this PR as well.

[ditman]

Updated branch to fix merge conflicts.

@chunhtai I've seen you've made some comments after LGTMing. Please, take another look, and if you're still good with this landing, set the autosubmit label (else withdraw your approval, please!) :P

[ValentinVignal]

@chunhtai @hangyujin is there anything else I need to do ?

[ValentinVignal]

@chunhtai @hangyujin If the code looks good to you, do you think we can go through with this PR?

[ValentinVignal]

@chunhtai @hangyujin , the check submit-queue keeps failing on this PR (and not on https://github.com/flutter/packages/pull/3462 for example), am I supposed to do something about it?

[chunhtai]

@ValentinVignal no that means the current tip of tree is broken, it would be us to fix it. No action item on your side.

[auto-submit[bot]]

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