-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[go_router] Use the correct configuration to build the state passed to the onExit
#6623
[go_router] Use the correct configuration to build the state passed to the onExit
#6623
Conversation
@@ -233,7 +233,7 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList> | |||
exitingMatches.length - 1, | |||
context: navigatorContext, | |||
matches: exitingMatches, | |||
configuration: configuration, | |||
// configuration: configuration, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my ! Sorry about that. I removed it in refactor: Remove commented code
); | ||
} | ||
return SynchronousFuture<bool>(false); | ||
} | ||
|
||
final FutureOr<bool> exitFuture = goRoute.onExit!( | ||
context, | ||
match.buildState(_configuration, configuration), | ||
match.buildState(_configuration, currentConfiguration), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the difference is whether we use the new RouteMatchList after the change vs old RouteMatchList?
I think for redirect we use the new RouteMatchList. it seems to previous implementation is more inline with the redirect API no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I maybe wrong here. do you have use case that using the old routematchList is more preferred? on the other hand, I plan to redesign redirect, so maybe we should just provide both before and after?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I found one in #6614. I was making go_router_builder
use onExit
in some tests failed https://github.com/flutter/packages/pull/6614/checks?check_run_id=24284539541.
The issue was when using .go()
. The onExit
had the state of the new page, not the previous page.
The test I wrote in this PR
'It should provide the correct path parameters to the onExit callback during a go'
fails without my changes.
On the other hand, I plan to redesign redirect, so maybe we should just provide both before and after?
Sure, do you want me to change something then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will need to redesign the onExit in the future. It also has issue with popscope and cupertinobackswipe. Thing may become a lot different after I resolve all these issues.
After thinking about it more, I think we should just keep it simple for now. I think what you have is fine.
…ct-configuration-in-on-exit # Conflicts: # packages/go_router/CHANGELOG.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ct-configuration-in-on-exit # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml
@hangyujin Is there something I need to do about this PR ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…passed to the `onExit` (flutter/packages#6623)
flutter/packages@1412041...fd714bd 2024-05-14 [email protected] [go_router] Use the correct configuration to build the state passed to the `onExit` (flutter/packages#6623) 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
…o the `onExit` (flutter#6623) While working on flutter#6614, I notice some issues with the state that is given to the `onExit` (see the run https://github.com/flutter/packages/pull/6614/checks?check_run_id=24284539541) This PR uses the correct configuration to build the state and pass it to the `onExit`
…o the `onExit` (flutter#6623) While working on flutter#6614, I notice some issues with the state that is given to the `onExit` (see the run https://github.com/flutter/packages/pull/6614/checks?check_run_id=24284539541) This PR uses the correct configuration to build the state and pass it to the `onExit`
While working on #6614, I notice some issues with the state that is given to the
onExit
(see the run https://github.com/flutter/packages/pull/6614/checks?check_run_id=24284539541)This PR uses the correct configuration to build the state and pass it to the
onExit
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.