feat: removeRoute now call didComplete #157725
Conversation
d6074ed to
01e9627
Compare
nate-thegrate
left a comment
There was a problem hiding this comment.
Maybe removeRoute should call didComplete internally but i didn't know what are the potential drawbacks.
I think this is a really good point.
If letting the Future hang forever is desired behavior for some reason, it probably makes sense to leave things as-is, given the workaround shown in #157505 (comment). Otherwise, I think it makes sense to unconditionally complete the future when the route is removed.
|
(PR Triage): Hey @EArminjon, thanks for sending another PR! Would you like to continue working on this change and address the feedback above? Thanks! |
|
Sorry for delay, I can try. |
cdf2970 to
2b2fdf9
Compare
5074b2e to
0eee47b
Compare
|
@Piinks done :) |
|
I didn't know if we must consider this as a breaking change. If someone created a logic around the fact that .push() will never be completed when using removeRoute, now it will be. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
0eee47b to
0094a59
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
0094a59 to
6408f86
Compare
| /// [NavigatorObserver.didRemove]). The removed route is disposed without | ||
| /// being notified. The future that had been returned from pushing that routes | ||
| /// being notified. The future that had been returned from pushing that route | ||
| /// will not complete. |
There was a problem hiding this comment.
It does feel a bit weird that removeRouteBelow is no longer consistent with removeRoute. Should we change all of these methods to complete their respective futures?
There was a problem hiding this comment.
pushAndRemoveUntil, restorablePushAndRemoveUntil, restorablePushNamedAndRemoveUntil and pushAndRemoveUntil could be supported but we need to add an extra logique like a callback to let developper choose to override the value of didComplete(result). if the callback is not provided we can fallback to null.
Maybe that can take place into an future pull request because that will also touch popUntil.
There was a problem hiding this comment.
PR edited to support removeRouteBelow.
There was a problem hiding this comment.
We can also go forward and try to support all method like replace, restorable etc.
There was a problem hiding this comment.
Must i support them all inside this PR with the risk to delay massively it or should i create a dedicated PR later ? (New unit test will be needed, lot of changes)
There was a problem hiding this comment.
Must I support them all inside this PR with the risk of a massive delay, or should I create a dedicated PR later?
That's a really good question.
I suppose my question would be: is it all right for removeRoute & removeRouteBelow to both complete their respective futures, but for none of the other similar methods to do this?
If we merge this pull request today and there end up being issues with migrating the similar methods, I think the API would be more inconsistent than it is now.
There was a problem hiding this comment.
pushAndRemoveUntil, restorablePushAndRemoveUntil, restorablePushNamedAndRemoveUntil and pushAndRemoveUntil could be supported but we would need to add extra logic, like a callback, to give the developer an option to override the value of
didComplete(result); if the callback is not provided we can fallback to null.
My opinion as of now is that it'd be best for each removed route to have its future completed, but we can just do didComplete(null) for now. If we want pushAndRemoveUntil to support completing with a value, that can be saved for a future PR.
Does that sound okay to you?
There was a problem hiding this comment.
That could be a great roadmap : first time didComplete(null) by default, next PR logic to well handle that everywhere.
There was a problem hiding this comment.
I tested quickly on my side, I can well update all these functions to support the result property, and when needed, a dedicated callback for whose related to popUntil and removeUntil. Will do that later in dedicated PR (if you and reviewer agree).
062127b to
010fc5e
Compare
010fc5e to
6f7737c
Compare
(edited 4 december) PR EDITED TO FIT LATEST COMMENTS
Developper may want to remove a route in the background while an other one is displayed.
To do so, developer can use removeRoute().
RemoveRoute() didn't resolve futures created by Navigator.push and so code which await for Navigator.push will never end.
By calling complete() inside removeRoute, futures are well resolved. In addition that also allow to pass some result through removeRoute.
Ex:
This PR aim to fix this issue : #157505
Pre-launch Checklist
///).