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

NavigatorObserver and Router API #124339

Open
chunhtai opened this issue Apr 6, 2023 · 6 comments
Open

NavigatorObserver and Router API #124339

chunhtai opened this issue Apr 6, 2023 · 6 comments
Labels
f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list team-framework Owned by Framework team triaged-framework Triaged by Framework team

Comments

@chunhtai
Copy link
Contributor

chunhtai commented Apr 6, 2023

The Problem

The NavigatorObserver was original built for Navigator API. After the Router API is introduced, we started to face several issues

  1. It can only work with one Navigator.

It is common to have nested navigators( or even no navigator) with Router API. A single navigation in Route API may trigger 0~n navigators to push/pop routes. Even if we somehow listen to all navigators, it may still be impossible to piece the notifications together.

  1. NavigatorObserver API is too opinionated

Router can achieve things like complete replace route stack in one navigation, there isn't a corresponding API in NavigatorObserver to record the event. It may produce a bunch of didPush/didPop/didReplace that are hard to make sense.

One extreme cases is something like CupertinoTabScaffold. The navigations in between tabs are achieved by using IndexedStack to move different screens around. In this cases, none of the NavigatorObserver will fire. We encounter this issue when we implements flutter/packages#2650

Proposal

A new observing system that based Router API. I don't have much detail yet, one initial idea is to have an observer that observe the Configuration

We can set the generic Type to Diagnotics. so that other third_party router package would need to make sure their Configuration implements the subtype in order to use the observer.

Things to consider

  1. HeroController should be migrated away from NavigatorObserver Allow Hero widgets to have transitions within the same screen #54200
  2. Existing analytics tools needed to be migrated
  3. Possible backward compatibility with NavigatorObserver
@chunhtai chunhtai added framework flutter/packages/flutter repository. See also f: labels. f: routes Navigator, Router, and related APIs. labels Apr 6, 2023
@chunhtai
Copy link
Contributor Author

chunhtai commented Apr 6, 2023

@goderbauer @Hixie I am curious if you have any thought on this.

cc @Milad-Akarie, for auto-router
cc @tomgilder, for routemaster
cc @slovnicki, for beamer

@goderbauer
Copy link
Member

I am not a big fan of the current NavigatorObserver API, so if we can replace it with something better, I am all for it! For any proposal it would be interesting to study how the existing use cases (mainly Heroes and analytics) can be implemented with it.

@chunhtai
Copy link
Contributor Author

chunhtai commented Apr 6, 2023

cc @johnpryan

@slovnicki
Copy link

I agree with the idea of implementing a new observing system (e.g. RouterObserver) for Router API that listens to currentConfiguration. This way, we

  • could handle generic needs of all routing packages and solutions
  • will not break NavigatorObserver implementers

but the whole situation indeed is much complicated than it might seem at first. There are several things to consider furthermore

Multiple Routers

In this case, the RouterObserver could follow a similar logic as is for RootBackButtonDispatcher and ChildBackButtonDispatcher so that only the root observer gives the final notification out.

Developer Experience

Developers not using Router API should be able to just keep using NavigatorObserver, but developers who are currently using Router API with some NavigatorObserver will be confused about how and why to migrate. There is a case for using both RouteObserver and NavigatorObserver because we can use Navigator.push (without interacting with currentConfiguration) and Router API simultaneously, but having to specify both RouterObserver and NavigatorObserver in different places looks too disconnected.

From this perspective, it seems that RouterObserver could be implemented as a composite of

  • NavigatorObserver (which *should be passed down to Navigator) and
  • "CurrentConfigurationObserver" that will watch changes in currentConfiguration

This way the RouterObserver will notify of all relevant changes and it is up to developer to chose what information to use for further processing (analytic tools, etc.)

Now all developers that are currently using some NavigatorObserver can just wrap it with RouterObserver.
There is some hand-waving here, especially with "should be passed down to Navigator" which may not be trivial or obvious in general case, but should be quite straightforward for most third_party packages if RouterDelegate receives the RouterObserver containing NavigatorObserver.

Now that I've written this composite approach, I don't see any reason to prevent us from just having something like

RouterObserver with CurrentConfigurationObserver implements NavigatorObserver { ... }

with which the migration would be even easier - just some renaming and moving things around.

(I will leave the initial composite idea so we have a full train of thought)

currentConfiguration as a target for observer

We can consider adding a new property to RouterDelegate specificaly for RouterObserver to observe.

The motivation behind this is that currentConfiguration may not be the best choice for routing notification and most developers will want to see something like the output of RouteInformationParser.restoreRouteInformation.

@Milad-Akarie
Copy link

Milad-Akarie commented Apr 7, 2023

A new or an enhanced navigator observer is definitely needed for the Router Api.
My take on this is to have an abstract NavigationEventsObserver that can be used by any navigation widget/entity ( Navigator, Router, TabsRouter ... etc ), the observer should not care about the widget dispatching the even.

Considerations I have in mind

  • We should be able to observer both router-configuration changes and changes per navigators and sub navigators.
  • Router developers can add their own navigation events by extending NavigationEvent and have their navigation-widgets dispatch them e.g auto_route would have DidInitTabRoute and DidChangeTabRoute events.
  • All navigators/navigation-widgets should be able to dispatch events to the same NavigationEventsObserver instance ( navigator_state can be passed with the event )
  • One place to observe all navigation events from all navigation widgets.

The initial implementation idea

// something like _NavigatorObservation
abstract class NavigationEvent{}

// navigator push event
class NavigatorDidPush extends NavigationEvent{
  final Route<dynamic> route;
  final Route<dynamic> previousRoute;

  NavigatorDidPush(this.route, this.previousRoute);
}

abstract class NavigationEventsObserver{
  void onNavigationEvent(NavigationEvent event);
}

// possible  back-compatibility with current NavigationObserver
// also mapping events to method-calls for a simpler api for newer-implementations
class NavigationObserver extends NavigationEventsObserver{
  
  @override
  void onNavigationEvent(NavigationEvent event) {
    if(event is NavigatorDidPush){
      didPush(event.route, event.previousRoute);
    }
   // ... handle other navigator 1.0 events
  }

  void didPush(Route route, Route? previousRoute){}
}
  • The NavigationEventsObserver can be passed to the Router and have it dispatch a RouterConfigrationChangeEvent(prevConfig?,config)

  • The Navigator can dispatch new events like DidBatchRemove(routes) or DidBatchPush(routes)

  • Not sure how to handle HeroController.

@chunhtai
Copy link
Contributor Author

chunhtai commented Apr 7, 2023

We could also consider using Notification
and NotificationListener

It is currently only used for ScrollNotification, we can add a new subclass for NavigationNotification. The possible down side of it is that this do a linear walk on the ancestor widgets to reach the listener which may potentially expansive.

I am not too worry about HeroController, I think we will decouple it from navigator anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list team-framework Owned by Framework team triaged-framework Triaged by Framework team
Projects
None yet
Development

No branches or pull requests

4 participants