Uh oh!
There was an error while loading. Please reload this page.
Return value when pop - #3368
Conversation
NazarenoCavazzon
commented
Mar 3, 2023
@chunhtai Here it is |
| go_router: | ||
| path: .. | ||
| logging: ^1.0.0 | ||
| package_info_plus_web: ^2.0.0 |
| ## 6.3.0 | ||
| - Support for returning values on pop. |
There was a problem hiding this comment.
| -Support for returning values on pop. | |
| -Supports for returning values on pop. |
There was a problem hiding this comment.
please move the item under ## NEXT to be under 6.3.0 as well
| ```dart | ||
| onTap: () { | ||
| final bool? result = await context.push<bool>('/page2'); | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { |
There was a problem hiding this comment.
This was before context.mounted was implemented, and using context after the value was returned lead to some dirty context problems, but we can take it out
| final _NavigatorStateIterator iterator = _createNavigatorStateIterator(); | ||
| while (iterator.moveNext()) { | ||
| if (iterator.current.canPop()) { | ||
| iterator.matchList.last.completer?.complete(result); |
There was a problem hiding this comment.
Instead of exposing the completer directly, can you add a method in RouteMatch something like didComplete()? and let ImperativeRouteMatch to override the method to call complete on the completer
| required String parentSubloc, // e.g. /family/f2 | ||
| required Map<String, String> pathParameters, | ||
| required Object? extra, | ||
| Completer<dynamic>? completer, |
There was a problem hiding this comment.
If you use method this property should be in ImerativeRouteMatch, and instead of dynamic, it should use generic type T
| error: matches.last.error, | ||
| pageKey: pageKey, | ||
| matches: matches, | ||
| completer: completer, |
There was a problem hiding this comment.
Why not let ImperativeRouteMatch to create the completer itself.
| required super.error, | ||
| required super.pageKey, | ||
| required this.matches, | ||
| super.completer, |
There was a problem hiding this comment.
Somewhere in the doc should mention about the completer and future and what it meant.
also, please remove the todo for me. Thanks in advance
chunhtai
commented
Mar 3, 2023
Since this is not really a breaking change, so you don't need to write a breaking change doc. Consider moving the snippet in the doc into the example folder |
NazarenoCavazzon
commented
Mar 3, 2023
let me know if there's another change to make |
| - Supports for returning values on pop. | ||
| ## NEXT |
There was a problem hiding this comment.
I meant the ##Next section should be merged in 6.3.0
There was a problem hiding this comment.
## Next meant whoever publish the next version will need to publish the changes in##NEXTas well
| } | ||
| /// Completes the promise returned by [GoRouter.push]. | ||
| void complete([T? value]) { |
There was a problem hiding this comment.
the entire completer should be move to ImperativeRouteMatch. This can just be a empty implementation for subclass override
There was a problem hiding this comment.
also maybe change this to didComplete. it is a special keyword in the framework, that has specific meaning
| throw MatcherError('Unexpected route type: $route', restLoc); | ||
| } | ||
| /// Completes the promise returned by [GoRouter.push]. |
There was a problem hiding this comment.
This should document what the purpose of this method
| /// ```dart | ||
| /// context.pop(true); | ||
| /// ``` | ||
| void complete([dynamic value]) {} |
There was a problem hiding this comment.
why optional? I think it can be Object? value.
There was a problem hiding this comment.
I meant this method should be call didComplete.
void didComplete(Object? value) {}
| throw MatcherError('Unexpected route type: $route', restLoc); | ||
| } | ||
| /// Completes the promise returned by [GoRouter.push], allowing comunication |
There was a problem hiding this comment.
Maybe:
Called when the corresponding [Route] associated with this route match is completed.
| /// Completes the promise returned by [GoRouter.push], allowing comunication | ||
| /// between pages. | ||
| /// | ||
| /// If the promise has already been completed, this method does nothing. |
There was a problem hiding this comment.
promise is not a keyword in dart. I think you meant future. Eitherway, this is implementation detail, we should focus on when/why this method is called.
| /// ```dart | ||
| /// context.pop(true); | ||
| /// ``` | ||
| void complete([dynamic value]) {} |
There was a problem hiding this comment.
why optional? I think it can be Object? value.
| /// The future of the [RouteMatch] completer. When the future completes, this | ||
| /// will return the value passed to [complete]. | ||
| Future<T?> get future => _completer.future; |
There was a problem hiding this comment.
The problem is we need to return this completer future in the push function, if we make it private we cannot return it.
There was a problem hiding this comment.
You can access private method as long as the class is in the same file, so it should be fine
| /// ```dart | ||
| /// context.pop(true); | ||
| /// ``` | ||
| void complete([dynamic value]) {} |
There was a problem hiding this comment.
I meant this method should be call didComplete.
void didComplete(Object? value) {}
| if (configuration.matches.last is ImperativeRouteMatch) { | ||
| configuration = | ||
| (configuration.matches.last as ImperativeRouteMatch).matches; | ||
| (configuration.matches.last as ImperativeRouteMatch<dynamic>).matches; |
There was a problem hiding this comment.
we should avoid using dynamic as much as possible unless you are handling json.
chunhtai
left a comment
There was a problem hiding this comment.
LGTM, just some nits and change log
| /// The future of the [RouteMatch] completer. When the future completes, this | ||
| /// will return the value passed to [complete]. | ||
| Future<T?> get future => _completer.future; |
There was a problem hiding this comment.
You can access private method as long as the class is in the same file, so it should be fine
| _completer.complete(value as T?); | ||
| } | ||
| /// The future of the [RouteMatch] completer. When the future completes, this |
There was a problem hiding this comment.
| /// The future of the [RouteMatch] completer. When the future completes, this | |
| /// The future of the [RouteMatch] completer. | |
| /// | |
| /// When the future completes, this |
There was a problem hiding this comment.
Didn't know about private methods being accessed if in the same file, really cool!
| - Supports for returning values on pop. | ||
| ## NEXT |
chunhtai
commented
Mar 3, 2023
and the analyzer is not happy |
NazarenoCavazzon
commented
Mar 3, 2023
@chunhtai fixed the change log, almost forgot. |
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: HBS <30705623+MrHBS@users.noreply.github.com>
booooohdan
commented
Mar 15, 2023
Waiting for resolving this PR |
chunhtai
commented
Mar 23, 2023
It looks like there is some conflict, can you rebase off the latest main? |
auto label is removed for flutter/packages, pr: 3368, Failed to merge pr#: 3368 with Pull request could not be merged: Pull Request is not mergeable. |
NazarenoCavazzon
commented
Mar 23, 2023
Done |
| @@ -1,5 +1,6 @@ | |||
| ## 6.3.0 | |||
| - Supports returning values on pop. | |||
There was a problem hiding this comment.
Updated to 6.5.0
There was a problem hiding this comment.
It looks like the conflict is back :(
There was a problem hiding this comment.
Yep, I'm resolving them rn
There was a problem hiding this comment.
Everything is a mess hahahaha, I'll rebase it to main and made the implementation again 🙃
There was a problem hiding this comment.
@chunhtai now there shouldn't be any more conflicts
tomassasovsky
commented
Mar 23, 2023
grande naza! |
NazarenoCavazzon
commented
Mar 23, 2023
Niceeeeeeeeee |
Return value when pop
Return value when pop
Return value when pop
In this PR I'm modifying the push and pushNamed by making them return a Future<T?>, and using completers to make the users able to return an wait values from within pages.
Fixesflutter/flutter#99663.
Fixesflutter/flutter#107217
Fixesflutter/flutter#100969
This PR will not conflict with older versions of go_router when updated.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto 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.