Any thoughts on implementing Router.neglect?

[nullrocket]

We have a homegrown router that already that is remarkably similar to go_router, probably because we started with navigator 2.0 and never used the original navigator API, but it would be nice to eventually move to something someone else maintains :)

On the web I often want to navigate with no browser history created, I get tired of fighting the browser navigation buttons! I still want the deep links and the browser URL changes though. I just want a bit more control over history.

We solve this by wrapping notifyListeners with Router.neglect I did a quick POC to get it working with go_router.

I'm wondering if this is a desirable feature for go_router? It wouldn't be a breaking change as implemented I suppose.

  // Add parameter to neglect history.
  void _safeNotifyListeners({bool neglectHistory = false}) {
    // this is a hack to fix the following error:
    // The following assertion was thrown while dispatching notifications for
    // GoRouterDelegate: setState() or markNeedsBuild() called during build.
    if (neglectHistory) {
      WidgetsBinding.instance == null
          ? Router.neglect(_aBuildContext, notifyListeners)
          : scheduleMicrotask(() => Router.neglect(_aBuildContext, notifyListeners));
    } else {
      WidgetsBinding.instance == null ? notifyListeners() : scheduleMicrotask(notifyListeners);
    }
  }

  // add flag to methods that call _safeNotifyListeners.
    /// Navigate to the given location.
  void go(String location, {Object? extra, bool neglectHistory = false}) {
    _log('going to $location');
    _go(location, extra: extra);
    _safeNotifyListeners(neglectHistory: neglectHistory);
  }

The main issue is Router.neglect needing the build context in places where its not in scope yet, I just hacked something quick to grab it on build, but haven't reasoned out a better way yet.

  late BuildContext _aBuildContext;

  Widget _builder(BuildContext context, Iterable<GoRouteMatch> matches) {
    _aBuildContext = context;
   ...
   ...

Edited this because the above isn't the full solution. You also have to wrap anywhere *pages[] is changed in Router.neglect. While I appreciate that go_router itself doesn't necessarily want to involve itself in managing history its implementation seems to be making it impossible to do outside of of go_router internals.

[csells]

This seems interesting, @nullrocket.

@lulupointu I know you've done some work with the browser history in VRouter. What are your thoughts on something like this in GR?

[lulupointu]

VRouter does not use the RouteInformationParser API at all (and instead re-implement the browser communication logic) so I don't have much at the top of my head.

However if I might add my inexperienced opinion, I think if we can we should strive to make Router.neglect work instead of re-implementing a different logic. We should try to access if that's possible. @nullrocket any thoughts on this? Have you looked at GoRouter code to see where the pages are changed?

[nullrocket]

@lulupointu yes, I have it working like I need it to on a local branch of go_router. It's not a complicated change, however there is the need for the build context from GoRouterDelegate build that I'm unsure how to implement.

I'm doing this right now, but it doesn't feel right.

https://github.com/csells/go_router/blob/ac6e499b21c8e42ebcf84569e07d5d1bd7853104/go_router/lib/src/go_router_delegate.dart#L597

 late BuildContext _aBuildContext;

  Widget _builder(BuildContext context, Iterable<GoRouteMatch> matches) {
    _aBuildContext = context;

Also I disable history globally, but perhaps per navigation method invocation would be more flexible and allow building your own history stack on top of go_router. In my experience there is very little reason to mess with history at this level except for the web though and global off / on is all I cared about when I made the changes.

[lulupointu]

I'm doing this right now, but it doesn't feel right.

Oh yes this is wrong but the right way to do it has been discussed and is painful. @csells here is another example of where this might be useful. That's the 3rd use cases we see. Maybe if there is enough of those we might have the motivation to implement it :smile:

Also I disable history globally

What do you mean ? You don't use Router.neglect ?

I have it working like I need it to on a local branch of go_router

I can't find it on your github. If you do use Router.neglect I would be very interested to see your implementation.

[nullrocket]

@lulupointu Here you go. https://github.com/nullrocket/go_router/tree/router_neglect This works, but no testing beyond "it works for me"

I implemented a default for Router.neglect so it could be set "globally" meaning it affects all navigation made via go_router.

I also implemented a per navigation method. I modified the examples in my branch so you can see the two different ways of controlling it.

This is an example of per navigation method. https://github.com/nullrocket/go_router/blob/a7eff08ddfde26d17e711e08696b6deafb4db615/go_router/example/lib/main.dart#L44

This is setting setting it globally.

https://github.com/nullrocket/go_router/blob/a7eff08ddfde26d17e711e08696b6deafb4db615/go_router/example/lib/books/main.dart#L38

[nullrocket]

Was a solution to the build context issue discussed previously? I hit the same problem in my own router and just did the easy thing, would be good to look at the proposed "right way" :)

[nullrocket]

Just a note on this change, it's non-breaking.

[lulupointu]

I see. Thanks for the code :blush:

If we had to integrate it into GoRouter I think I would rather having it as something supported rather than integrated:

// Integrated
context.go('/page2', routerNeglect: true)

// Supported
Router.neglect(context, () => context.go('/page2'))

The difference being that we don't cluster the go method we optional parameters.

Another option might be:

GoRouter.of(context).neglect('/page2')

Which is in between.

I think I prefer Router.neglect(context, () => context.go('/page2')) just because in an ideal world every navigation package would be able to support it (since its a Flutter mechanism and every package is built on top of Flutter) which would make it easier for devs to switch between packages.

[nullrocket]

Unfortunately that won't work, at least it didn't work for me. I think if _safeNotifyListeners didn't have the scheduleMicrotask call it might.

[lulupointu]

Was a solution to the build context issue discussed previously? I hit the same problem in my own router and just did the easy thing, would be good to look at the proposed "right way" :)

Here is the basic idea (to be expanded upon).

Btw your navigation package is not searchable is it? I like to see the different implementations :wink: . Is it based on the same ideas/principles as GoRouter?

[nullrocket]

But the last option could, but then instead of optional params you will have more methods? I personally lean towards the params.

We would need to implement

neglect
neglectNamed
neglectPushNamed
neglectPush

[lulupointu]

Unfortunately that won't work, at least it didn't work for me. I think if _safeNotifyListeners didn't have the scheduleMicrotask call it might.

I know it doesn't. I was just saying that if it were to be supported this is what I would rather make it look like :wink:

Anyway we need to hear what @csells thinks about this

But the last option could, but then instead of optional params you will have more methods? I personally lean towards the params.

Oh you are right. Also I just look and it turns out that go does not have many params so yes option 1 or 2 would be fine I guess. Though routerNeglect is not the best name imo (isReplacement, isHistoryReplacement ?).

[nullrocket]

It's not searchable, I will try to get it up soon, it is pretty experimental, but it is pretty similar to go_router at it's core.

There are two differences, one is minor, we use a Trie with wildcard nodes to parse the routes and build page stacks.

The other difference is it has the concept of "outlets", basically routeable widgets, You can define "outlets" on a route and pass a widgets to the outlets in the route definition just like pages. They get build and inserted anywhere you have an outlet widget that matches the route on your screen.

I don't know if you are familiar with ember.js but thats where I got the concept. Not exactly the same... but a similar concept.

here is a link if you are interested in the outlets concept. https://guides.emberjs.com/release/routing/rendering-a-template/

[nullrocket]

LOL naming is hard, I went with routerNeglect just to link the concept to what the flutter navigator was calling it to keep the concepts simple, if you see routerNeglect you can link that to Router.neglect as you think about the code.

[nullrocket]

Is there a reason that _safeNotifyListerns calls scheduleMicrotask , I haven't ever had to do that and if it could be removed that would probably make it possible to do the first and have zero need to modify go_router. Although not having it integrated makes for very noisy code wrapping everything. I would not enjoy that.

[lulupointu]

Thanks I will definitely have a look. Oh ok I see what you mean by outlets. It's a bit like AutoRoute NavLink but generalized.

LOL naming is hard

Certainly the hardest, despite (or in spite) of how much we like to joke about it :wink:

I went with routerNeglect just to link the concept to what the flutter navigator was calling it to keep the concepts simple, if you see routerNeglect you can link that to Router.neglect as you think about the code.

I see but I think people are using GoRouter to flee the Router API. Therefore I think that it is highly likely that they don't know about Router.neglect. Just my opinion though, I have no concrete data.

Is there a reason that _safeNotifyListerns calls scheduleMicrotask , I haven't ever had to do that and if it could be removed that would probably make it possible to do the first and have zero need to modify go_router.

It's a hack to solve the issue mentioned in _safeNotifyListeners body:

// this is a hack to fix the following error: // The following assertion was thrown while dispatching notifications for // GoRouterDelegate: setState() or markNeedsBuild() called during build.

I think this could be removed through careful asserts but @csells would know better why this can/cannot be avoided

Also have you tried removing scheduleMicrotask just to see if Router.neglect works in this case?

Although not having it integrated makes for very noisy code wrapping everything. I would not enjoy that.

You could easily create an extension for your project if you want to use it extensively:

extension GoNeglectBuildContext on BuildContext {
  void goNeglect(String location, {Object? extra}) => Router.neglect(
        this,
        () => go(location, extra: extra),
      );
}

// Usage
context.goNeglect('/page1');

Something along these lines

[nullrocket]

I think I will dig into the scheduleMicrotask thing, I've removed it and will see if i can reproduce the issue noted.

[lulupointu]

This is the only reference I could find of an issue. So I guess it was added to please some tests :smile: . @csells should be able to explain more ^^

[nullrocket]

@lulupointu I removed scheduleMicrotask, the tests are fine and I can't reproduce the issue so I moved on to seeing if wrapping externally would work. It does, with one minor issue, if the initial route redirects that route is put into history, I'm sure this can be fixed.

https://github.com/nullrocket/go_router/tree/remove_schedulemicrotask

Just to save some time verifying that it works I modified GoRouterHelper

https://github.com/nullrocket/go_router/blob/a8a81bcd47445e95b01a6cb1444f23e155919e6d/go_router/lib/go_router.dart#L21

I also modified the default example and the books example to not use history. The default example has no redirects on the route definition and works correctly, the books example has the auth listener that causes a refresh and does add an initial history entry, but no further history.

So other than removing scheduleMicrotask the only other thing to do is track down how to wrap redirects happening in refresh due to an external trigger I believe.

[nullrocket]

@csells Is there any way to reproduce the error that using scheduleMicrotask solved? I wouldn't mind tackling removing it if I can reproduce it.

[lulupointu]

If I had to guess I would say that the error it solve is calling context.go inside setState (or build). But I haven't checked

[csells]

@nullrocket @lulupointu The eng on the Flutter teams says I shouldn't get that error, either, but I did and I don't remember where. I think if you take out the microtask code and can run all of the examples through their paces (as well as the tests) then we should be good. It's a hack that I'd be happy to remove if it's no longer needed.

As far as supporting neglect goes, do we want that to happen on each nav or can we set a parameter on the GoRouter and just have magic happen? Also, what does happen when neglect is used? Does the browser's back button behavior change in some way?

[nullrocket]

I have taken it out and ran the tests and put all the examples through all their screens with no errors.

I think both would be nice to have, a default that lets it be off or on for all calls and a per nav option. it may be possible for this to live outside go_router, but I need to look at the refresh that happens when an external notifier like the auth example happens, since it isn't wrapped in Router.neglect it creates history, that may need some thinking to do outside of go_router.

So what happens with neglect is no history entry is created for the browser, that changes the back button behavior since there is no history to go back to ... or forward to, but the URLs do change and deep linking continues to work as do the flutter native navigation stacks, a detail view with a back arrow in the navbar header automatically enabled via flutter will still go back (pop) the stack.

Probably an unpopular opinion but I would really prefer the default be no history on the web. While this may seem like a loss for those vocal about the web feeling like the web, it definitely isn't! Fighting the web's native behavior around what should happen with history entries vs what a flutter or any complex application expects is impossible to sync given current browser API's so I would say 100% of the time on every web project I have worked on not having the back/forward buttons work would be the best choice... often arrived at by wasting a lot of time trying to satisfy a requirement that just won't work.

Anyway, if we can arrive at an API around this I would be happy to put some work into it and do a pull request.

I believe these are the choices for API

1. Router.neglect is integrated and the user only needs to set a param on navigation calls and a default for global behavior. This is the easiest to ensure all routing is captured and history discarded.
2. Router.neglect is external and the user needs to choose to discard history at each call site, @lulupointu had an example of a helper that would make that easier.
3. Something I haven't imagined?

I would lean towards 1. I get to disable it globally and I also get to continue to use the context.go method instead of something like context.goNeglect. Less cognitive overload at least from my point of view.

[csells]

I think a partial implementation of #1 would make sense, i.e. just a global option. I'm not quite sure I see the use case for choosing to add/not add browser history on a per-route basis.

[nullrocket]

@csells Ok, I can work something up. Would you like a separate pull request in the mean time removing scheduleMicrotask? I have tried to trigger the issue and haven't been able to, I don't know of any way to ensure it is a solved issue though.

The global option to disable history will work whether it is removed or not though.

[csells]

A separate PR for each of the global neglect implementation and the micro-task removal would be much appreciated. Thanks, @nullrocket.

[csells]

published in v2.5.7