route.action() should not happen if previous onBeforeLeave cancels routing

[manolo]

It seems that execution of navigation methods in router is out-of-order. The new component to display is computed before event in the previous element is executed, it means that route.navigate is always executed, thus the element has been resolved before the method in the previous element might prevent navigation.

For Flow views, this is a critical problem because the new view is created before any signal might be sent to the server to prevent navigation.

I set some logs to prototype and this is the order of method calls.

Load the app pointing to categories:

>>>>>>>>>>>>>----- navigate /flow-category-list
>>>>>>>>>>>>>----- onBeforeEnter /flow-category-list

Navigate to reviews:

>>>>>>>>>>>>>----- navigate /flow-review-list
>>>>>>>>>>>>>----- onBeforeLeave /flow-category-list <- /flow-review-list
>>>>>>>>>>>>>----- onBeforeEnter /flow-review-list <- /flow-category-list

[vlukashov]

A bit of background to explain the current behavior.

The way Router currently works is that for every navigation it first resolves the new pathname to a chain of routes, compares the new routes chain with the previous one and renders the diff.

The onBeforeLeave callback is called when at least the parent layout of the the new routes chain is known, and the nested routes of the new chain are being resolved.

When resolving the parent layout of the new routes chain Router executes all route.action() methods it as it traverses the routes tree (as a part of the router.resolve() call). At this point it does not call the onBefore callbacks on the currently rendered components yet.

It does make sense to first check if the currently rendered view allows the navigation, before even starting to resolve the new routes chain. Currently that does not happen at the moment.

That might require changes to the route resolution flow so that the first parent layout and the full routes chain are resolved following the same rules (they are slightly different now)

[platosha]

This is tricky, because both onBeforeLeave and action can prevent the navigation.

If action is invoked first, and then some onBeforeLeave prevents navigation, this means we had an unnecessary action, what we have now.

If onBeforeLeave is invoked first, then some action prevents navigation, this means we had an unnecessary onBeforeLeave lifecycle callback invoked.

So there are two kinds of evil here. It seems we ended up with the first one, because the current implementation assumes that having wrong lifecycle callbacks because of tricky actions is more harmful, than the opposite.

As @manolo points out in the description, unnecessary action calls that return an element also have an evil impact. We discussed this with @vlukashov and concluded, that it would be preferable to mitigate the impact of extra action calls rather than to change the resolution / lifecycle order.

To minimise the DOM work produced by extra action calls, let us introduce a way for actions to postpone rendering with an async callback. Here is a usage draft:

router.setRoutes([
  {path: '/path', action: (_, commands) =>
    commands.render(
      async root => {
        const element = document.createElement('foo');
        root.appendChild(element);
        return element;
      }
    )
  }
]);

Such a callback would only be called after the chain is fully resolved, i. e.:

• all the actions are invoked,
• all their corresponding redirects were applied. If some action redirects away from the current action '/path', the rendering callback of the action is not invoked
• If navigation was prevented in any of the action calls, none of the render callbacks are invoked.

[platosha]

We have thought through the order of lifecycle callbacks with the team, and concluded that the existing order is good. From the standpoint of saving on client-server round trips, having resolution, which invokes actions, before calling onBeforeLeave makes sense.