Skip to content
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] Return a value on pop #2416

Closed

Conversation

NazarenoCavazzon
Copy link
Contributor

@NazarenoCavazzon NazarenoCavazzon commented Aug 4, 2022

In this PR I'm adding 2 new methods, pushAsync and pushNamedAsync, that allows you to wait for a value when the widget pushed pops, like in navigator 1.0.

Fixes flutter/flutter#99663.
Fixes flutter/flutter#107217
Fixes flutter/flutter#100969

This PR will not conflict with older versions of go_router when updated.

Here's the link of the document with the changes: https://docs.google.com/document/d/1bcPV8yNAETmlxTFTMpO_JcJ7-yMoF8qaCPa884hFmXA/edit?usp=sharing&resourcekey=0-73tg8sZOVf0YXBmz-MLwQw.

Pre-launch Checklist

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

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

@NazarenoCavazzon NazarenoCavazzon changed the title Feat/return value on pop [go_router] Return a value on pop Aug 4, 2022
@Rodsevich

This comment was marked as off-topic.

@jorger5

This comment was marked as off-topic.

@NicoCieri10

This comment was marked as off-topic.

@Matias-Olivier

This comment was marked as off-topic.

To make it easier to use, and also more like Navigator 1.0, I changed the dynamic value to T, T extends Object? so in that way we will always expect a nullable value from a pushAsync or pushNamedAsync. The tests has been updated.
@NazarenoCavazzon
Copy link
Contributor Author

NazarenoCavazzon commented Aug 4, 2022

I updated the PR, so now instead of doing:

final result = await context.pushNamedAsync(routeName) as bool?;
context.pop(someValue);

you can do:

final result = await context.pushNamedAsync<bool>(routeName);
context.pop<bool>(someValue);

Or even:

bool? result = await context.pushNamedAsync(routeName);

In both cases the returned value will be nullable.

@NazarenoCavazzon
Copy link
Contributor Author

Now because we save the completer in the RouteMatch it'll not break when pushing the same screen over and over again

@chunhtai
Copy link
Contributor

We should not merge this until we figure out this how to support imperative API moving forward
flutter/flutter#99112. Closing for now

@chunhtai chunhtai closed this Aug 18, 2022
@tatsuyuki25
Copy link

wait this issue...

@chunhtai
Copy link
Contributor

chunhtai commented Sep 2, 2022

I know this is a missing feature, and probably a big one. But we would like to do a refactoring on the imperative API which may make this fix obsolete or hard to maintain. Sorry for the inconvenience.

@ismanong
Copy link

One should be patient, for the sake of a good API, where flaws are temporary and respectable and rigorous.

@NazarenoCavazzon
Copy link
Contributor Author

NazarenoCavazzon commented Nov 17, 2022

@chunhtai when do you think we can add this feature to go_router? I have a branch with the implementation updated and all the tests ready to be reviewed. Also that code has been in production in multiple apps working flawlessly.

@paintingStyle
Copy link

@chunhtai您认为我们什么时候可以将此功能添加到go_router?我有一个分支,其实现已更新,所有测试都已准备好接受审查。此外,该代码已经在多个应用程序中完美地工作。

I currently need to return to the previous page to carry parameters. How should I do it before you merge it? I don’t know how to do it

@NazarenoCavazzon
Copy link
Contributor Author

I have a package called go_router_flow in pub, it's updated to the last go_router update, returning values work the same as navigator 1.0

@ChopinDavid
Copy link
Contributor

We should not merge this until we figure out this how to support imperative API moving forward
flutter/flutter#99112. Closing for now

So we can't get async push and go until removeRoute, popUntil pushAndRemoveUntil, and replace are implemented? Why?

@NazarenoCavazzon
Copy link
Contributor Author

@chunhtai any news?

@chunhtai
Copy link
Contributor

chunhtai commented Jan 5, 2023

I think this is already fixed when i refactor the pop.
See https://pub.dev/documentation/go_router/latest/go_router/GoRouter/pop.html

@Katekko
Copy link

Katekko commented Jan 18, 2023

I think this is already fixed when i refactor the pop. See https://pub.dev/documentation/go_router/latest/go_router/GoRouter/pop.html

Wich version this will be merged? I can't find this pop in API (6.0.1)

@maironLucasSlz
Copy link

Right, so now pop can come with a value after @chunhtai refactor. But how can I retrieve this value? Since both push and go return void and void can't be used? Someone understand how this will work?

@chunhtai
Copy link
Contributor

chunhtai commented Mar 3, 2023

hmmm, I mainly added for show dialog and imperative API. We should probably expose return future for gorouter push.

@chunhtai chunhtai reopened this Mar 3, 2023
@chunhtai chunhtai self-requested a review as a code owner March 3, 2023 17:22
@chunhtai chunhtai closed this Mar 3, 2023
@NazarenoCavazzon
Copy link
Contributor Author

@chunhtai just to let you know, this PR is outdated. If you want I can make the update. I have another branch with a better/cleaner implementation of this.

@chunhtai
Copy link
Contributor

chunhtai commented Mar 3, 2023

Sure feel free to open a new pr and pin me to review. I think what's left is the push need to return a future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet