Skip to content

[go_router] ShellRoute will merge GoRouter's observers #9436

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

huanghui1998hhh
Copy link

ShellRoute currently does not trigger observers defined in GoRouter, which is unexpected behavior.

Fixes flutter/flutter#112196

Pre-Review Checklist

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

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2 3

/// Only effective for `observers` passed into `GoRouter`.
///
/// Defaults to `false`.
final bool mergeObservers;
Copy link
Contributor

Choose a reason for hiding this comment

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

why make this a per route setting instead of a global setting in goRouter?

Copy link
Contributor

Choose a reason for hiding this comment

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

also what if it is a shellroute nested inside another shell route, if this is true, should the innershellroute notify the observer of outer shellroute?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe we should use Navigator.maybeOf(context).widget.observers to merge observers and keep the mergeObservers property for each ShellRoute.

This ensures that the case of ShellRoute nesting ShellRoute is handled correctly, but there is one exception: when ShellRoute is nested under a Navigator that is not built by ShellRoute. Maybe we should ignore this case.

The reason for adding mergeObservers property to each ShellRoute is to allow users to decide that a specific ShellRoute should not merge observers. Defining it as a global setting in GoRouter would make maintaining this case difficult.

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

Successfully merging this pull request may close these issues.

[go_router] ShellRoutes seem to cause NavigatorObserver to not fire (5.0.1)
2 participants