Skip to content

Conversation

@koji-1009
Copy link
Contributor

@koji-1009 koji-1009 commented Jul 20, 2022

Restore the function to push the same page multiple times, which was supported in v3.1.1.

Fixes flutter/flutter#107045

Refs

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.

@koji-1009 koji-1009 marked this pull request as ready for review July 20, 2022 12:33
@koji-1009 koji-1009 changed the title [go_router] Fix a page pushed twice will fail the assertion [go_router] Allow any number of the same page on the stack Jul 20, 2022
Semapl3 added a commit to Semapl3/packages that referenced this pull request Jul 20, 2022
@ValentinVignal

This comment was marked as off-topic.

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started to think support the push in the declarative way may be the wrong decision. as soon as a page is pushed to the stack, the stack will not be in sync with the url any more.
In this particular use case where you want to push the same page, does it make more sense to be a pageless route? that is, pushing use the Navigator.of(context). The difference is the pageless route will belong to the top most page route and will be move/remove with that page route

@koji-1009
Copy link
Contributor Author

On GitHub, after opening the top of the repository, you can open the list of pull requests and open the pull request details.
Also, using this PR as an example, clicking on the flutter:main branch link will open the top of the repository. A use case would be to implement this process in Flutter Web/Android/iOS.
At this point, we have the choice between context.go and context.push. The problem in this case is not go but push, so we want to back to the last open page.


If we don't want the delegate to have a Map, why not change the process so that the pageKey has a random suffix (e.g. UUID). Having state.pageKey as the Route key is the preferred method for most applications that use go_router.

@koji-1009 koji-1009 force-pushed the fix_page_twice_push branch from 3cbf18c to 12388eb Compare July 24, 2022 04:29
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some nitpicks

null,
);

goRouter.push('/a');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you do an await tester.pumpAndSettle() in between every push to verify it does not crash?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, I fixed it in 2ee5fd6.

@chunhtai chunhtai requested a review from johnpryan July 25, 2022 16:08
Copy link

@prince-kumar prince-kumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chunhtai
Copy link
Contributor

not sure why the check failed, can you rebase and try again?

koji-1009 and others added 2 commits July 26, 2022 08:57
Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com>
@koji-1009
Copy link
Contributor Author

flutter/plugins#6146

@stuartmorgan-g
Copy link
Collaborator

This change must not be landed until the underlying problem with 4.2.1 is fixed, or it will re-break everyone that was broken by 4.2.1 (since it'll publish a new version with the same bug).

@johnpryan @chunhtai please don't approve any more go_router PRs until the fix for the breakage is fixed, to avoid any accidents.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a request changes until the above has been resolved to ensure this doesn't accidentally land.

@stuartmorgan-g stuartmorgan-g dismissed their stale review August 1, 2022 12:09

The regression has been resolved

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 1, 2022
@auto-submit auto-submit bot merged commit b5d84d7 into flutter:main Aug 1, 2022
@koji-1009 koji-1009 deleted the fix_page_twice_push branch August 1, 2022 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: go_router

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[go_router] A page pushed twice will fail the assertion !keyReservation.contains(key)

6 participants