feat: Expose a route stream on RouterOutlets#1064
Conversation
jacobaraujo7
left a comment
There was a problem hiding this comment.
Thanks a lot for this, Robbie — the underlying need is real and well described. The Scaffold-inside-Scaffold case (chrome that sits beside a RouterOutlet, like a BottomNavigationBar in a shell) genuinely doesn't get updated by context.routeState(), because that only reaches widgets above/inside the active outlet subtree via the root RouteStateScope. So 👍 on spotting the gap.
That said, I'd like to request a few changes before we merge.
1. The emitted Uri diverges from what the outlet reports to the URL
_reportLocation() has always reported _stack.first.uri (the outlet's base sub-route) to the delegate — that's what becomes the URL. The new stream instead emits the path argument, which is the push/navigate target (_stack.last), so the stream value and the URL-reported value can disagree. On top of that, push('dashboard') accepts relative paths, so Uri.parse(path) can emit a relative Uri while the delegate gets the resolved absolute one. Consumers would get inconsistent values depending on which method they triggered.
2. Inconsistent across call sites
Some callers pass the explicit path (push, navigate, replace, popAndPushNamed, pushNamedAndRemoveUntil), while popUntil, _remove (the real pop) and the reseed in didChangeDependencies fall through to the _stack.last.uri branch. So pop/reseed emit a different shape than push/navigate. A pop happens to emit the new top by luck, but none of this is covered.
3. API shape goes against the v7 philosophy
Requiring GlobalKey<RouterOutletState> + currentState + addPostFrameCallback to observe routes reintroduces imperative State-grabbing that v7 deliberately moved away from (no global singleton, everything context-based). The idiomatic fix for the exact same problem would be to expose the outlet's current route reactively via an InheritedNotifier (mirroring what RouteStateScope already does for the root), so sibling chrome can do something like context.outletRouteState() and rebuild on its own — no GlobalKey, no Stream, no post-frame dance, and it stays consistent with _stack.first.uri and relative-path resolution.
If we also want an imperative stream for analytics-style listeners, that's fine to keep — but it should always emit the resolved absolute Uri (consistent with what the delegate gets), cover pop/popUntil/reseed, and come with tests.
4. Tests + docs
The checklist is unchecked: no tests for the new stream, no doc/ update, and the dartdoc comment has a typo ("notfiy"). New public surface needs at least a unit test and a /// doc.
Happy to help shape the InheritedNotifier approach if you'd like to go that route — I think it lands the same use case much more cleanly. Thanks again for digging into this!
| final List<_OutletEntry> _stack = []; | ||
| int _seq = 0; | ||
|
|
||
| /// Expose a event stream of the routes change |
There was a problem hiding this comment.
The stream emits the raw path argument here, which is the push/navigate target (_stack.last) and may be relative (push('dashboard')). But _reportLocation reports _stack.first.uri (the absolute base) to the delegate / URL. These two should not disagree — consumers will see a relative or last-route Uri that doesn't match the actual outlet location. Prefer emitting the same resolved absolute Uri the delegate gets.
|
|
||
| /// Reports this outlet's current base sub-route to the root delegate so the | ||
| /// URL reflects the outlet (a tab switch shows up). A push/pop leaves the | ||
| /// base unchanged, so it reports the same value and stays out of the URL. |
There was a problem hiding this comment.
_remove (the real pop path), popUntil, and the reseed in didChangeDependencies all call _reportLocation() with no path, so they emit _stack.last.uri while push/navigate emit the passed path. The emitted value's meaning changes depending on the operation. Whatever shape we settle on, it should be uniform across every mutation — and covered by a test.
|
Thanks for the feedback @jacobaraujo7. I will have a look into the InheritedNotifier approach. I had checked out the Flutterando Discord but I don't speak Portuguese so I couldn't contribute! |
Description
This adds a Uri Stream that can be listened to as the current
context.routeState()doesn't update everywhere and can be caught within apps that have Scaffold's within ScaffoldsChecklist
fix:,feat:,docs:etc).docand added dartdoc comments with///.example.Breaking Change
Related Issues