Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Add support for preloading branches of StatefulShellRoute (revised solution) #6467
[go_router] Add support for preloading branches of StatefulShellRoute (revised solution) #6467
Changes from 4 commits
d4f2d0d
fc00590
62b9894
126787f
b0a92bf
eb3db3b
c2eb9b1
5d93833
523bf75
ca3e898
6e6c39e
d27cf3a
ea48de0
9d68693
604744b
4bed9fd
a5f6600
1b85750
6b50167
2e631cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'd find it clearer if the last line was uncommented as
preload: false
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.
You may be right, I'll consider this.
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.
just realized somewhere in this method should probably remove entries in _branchNavigators the are no longer in the StatefulShellRoute?
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.
You're right, that is definitely necessary to do now. Will have a look at it.
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.
@chuntai, I added a fix for removing old entries, but when attempting to write a test case, I ran into issues with duplicate GlobalKeys (_shellStateKey) when updating the dynamic RoutingConfig. Or - I should be clear - at first the test case worked, but not because of the code I added, but instead because the entire StatefulShellRoute was replaced/reloaded. But when I added the possibility to set the
GlobalKey<StatefulNavigationShellState> _shellStateKey
to maintain state, I started to get this error.When trying to troubleshoot it, I started to wonder whether dynamic RoutingConfig full supports ShellRoutes, at least with state. Added some test cases to test this, and it seems simple StatefulWidgets in a simple routing scenario works, but when adding a (regular) ShellRoute with a stateful shell I also get and issue with duplicate GlobalKeys.
Not sure if I just missed something, but I anyway added a gist with the test cases here: https://gist.github.com/tolo/922fd838cdea30b17121533b0012eda4
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.
can you file an issue for it? looks like a bug
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.
is the fix in this pr already?
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.
It is now. 😊 Note though that I disabled two tests that fail due to the issue I mentioned above.