Mithril.js: [rewrite] oninit not called when setting the same route

[veloce]

Description:

This is a follow-up of #1177 and #1178, but this time I clearly identified the problem.

The problem arises when calling m.route.set on the same route we are currently in. Same route is either /route or route/:param even if the route arg changes.
In that case oninit is not called. Since it is used to load data, new data will never load, thus the screen never updates.

Steps to Reproduce:

https://jsbin.com/tiwiyuqifo/edit?js,console,output

m.route(document.body, '/', {
  '/': {
      view : () => m("div", "Home")
  },
  '/game/:id' : {
    oninit( vnode ){
      console.log(`oninit ${vnode.attrs.id}`);
      this.id = vnode.attrs.id;
    },
    view() {
      return m( 'div', 'game ' + this.id);
    }
  }
});

setTimeout(() => m.route.set('/game/1234'), 2000);
setTimeout(() => m.route.set('/game/5678'), 5000);

Expected:

We should see 'game 1234' then 'game 5678'.

Actual:

We only see 'game 1234'.

[pygy]

oninit is only called when the component is created, and in v1.x routing diffs the tree rather than rebuilding it from scratch.

Maybe you could tap into onbeforeupdate rather than (or in addition to) oninit, if necessary abstracting the common code in a distinct function?

[veloce]

Indeed, if I want to reroute the same route with a different id I could do something like:

'/game/:id' : {
    oninit( vnode ){
      console.log(`oninit ${vnode.attrs.id}`);
      this.id = vnode.attrs.id;
    },
   onbeforeupdate(vnode, old) {
      // when attrs are different it means we changed route
      if (vnode.attrs.id !== old.attrs.id) {
         this.id = vnode.attrs.id;
      }
   },
    view() {
      return m( 'div', 'game ' + this.id);
    }
  }

But something tells me, that it doesn't "feel right" to use onbeforeupdate to do this. It's like using a rather obscure lifecycle method (that is called on each redraw) to do something trivial, related to routing and not redraw. But maybe I'm wrong.

Moreover the onbeforeupdate trick doesn't address the problem when I want to reload the same page (see #1177). I have a use case for that in my app. On lichess tv, I'd receive a socket event telling me that a new game is to be shown, event that triggers a m.route.set('/tv') to reload the screen. As a workaround I could push the current game id to the tv url but it would not feel right as well. After all, reloading the same screen should be a valid use case, right?

I don't know the reason why now components are not rebuilt on route change, so I'm just throwing suggestions here. Maybe for a routing purpose we could have a new lifecycle method, like onreenter, even though, when saying that, I feel that it would break the coherence of the whole lifecycle events we have now and that are related to vnodes, not routing.

Actually my first impression here is that rebuild root components on route change is the simplest way to address this problem, but again, I don't know the implication of this in the rest of mithril.
So at least, imo, if this is the desired behavior, there should be something in the doc that explains this, because you can't just reuse your v0 controller as oninit in v1.0.

[pygy]

There's also the router hooks:

m.mount(el, default, {'/some_route':{
  resolve: function(...){ /* Called on route */ },
  render:function(...){ /* Called on every render */ }
}})

But that won't help you set the state of the component... What prevents you from using vnode.attrs.id in the view?

[veloce]

I use the vnode.attrs.id to load asynchronously data from the server. It can't be done in the view.

[pygy]

Then you could do it in the resolve hook, and pass the result of the request to the component as an attribute...

'/game/:id' : {
  resolve: function (render, args, path, route) {
    args.stuff = m.request({...})
    render({view: function () { return m(Game, args) }})
}

[veloce]

I see, this is an advanced usage of the router undocumented yet.

I understand now the code in api/router.js. And I understand better now the components differences between v0.2. They are indeed... components, that is: only related to vdom. Passing args to the root component from the router is the way to go, definitely. This is all for good of course, everything is much more coherent and well thought.

That being said the problem is also present in resolve hook. See this jsbin:
https://jsbin.com/diwalatuxi/edit?js,console,output

there is only one log "resolve 1234".

resolve is not called here because of this condition.

Imo, the router should only use the resolve api, and not the more magic one where one can supply either a component, or an object with resolve. This is harder to document this way and may lead to confusion. But this is only my 2 cents ;)

Also, I think the current migration guide may lead to confusion. Old components { controller: function() {}, view: function() {} } don't map to { oninit: function(), view: function() } because of that oninit is not being called the same way as controller.
I'll try to write something about it in the guide and send a PR if that may help.

[pygy]

The current api/router.js#19 for posterity...

I agree, current.path !== path may be better.

[veloce]

Yes but it still does not allow to reload the current page which is a use case. current.path !== path may be a solution, but then we need a way to force reload the page.

[lhorie]

current.path !== path may be better.

oh, that's a great catch, thanks for spotting that bug!

why now components are not rebuilt on route change

The reason why I'd like that to be the case is to avoid the need for the m.redraw.strategy API, which is fairly notorious for being confusing. With that being said, your use case is legit, imho, and should be supported.

In terms of how to actually fix this issue: one possible course of action would be to pass path as a key to the component being rendered, which would restore the recreate-from-scratch semantics from v0.2.x (except for when the path is exactly the same - more on that below)

But doing this in framework space would then create a need for something like m.redraw.strategy again, so I'm not super sold on that idea.

Alternatively, you could put a key on your component yourself (which would allow you to have m.redraw.strategy("diff") semantics on layout elements, but recreate-from-scratch semantics on an inner component.

The issue is that both of these cases still break for the use case of reloading the same route. I went to check how react-router does it, but apparently they think it's not a router concern. Gooling it for vue-router didn't help me much either :(

Gonna need to think about this a bit more carefully. (In the meantime, suggestions are more than welcome)

[isiahmeadows]

@lhorie Here's how I think it could be solved:

1. On a normal route change, clear and rebuild the tree entirely, kind of like m.mount(el, null) followed by m.mount(el, component). This would work similarly to m.redraw.strategy("all") in 0.2.
2. On a new kind of route change, allow a special type of routing that simply swaps the child component at the bottom and invokes a new redraw. This would work similarly to m.redraw.strategy("diff") in 0.2.

I don't know if it can be implemented easily or not with the current routing architecture, though.

[lhorie]

@isiahmeadows can you clarify what you mean by "normal" and "new kind of" route changes?

[pygy]

The resolve hook may be called unconditionally on route, with a modified signature:

resolve: function(render, args, current: {route, path}, previous: {route, path}){}

One could get 'all'-like behavior by using a counter as a key (as Leo said, either on the root or deeper down the tree), incremented when needed.

[veloce]

I think there are two different issues discussed here. The first is that there is no way to reload the data when one is either changing the same route with different arg, or reloading the same route.

The second is related to the decision, on route change, of whether one wants to recreate-from-scratch, or diff the view, and imho this issue is not a router one. It should be up to the app's author to decide what to do on route change, whether it is to reload data and trash the view, or to reload data and diff the view, or to do nothing...

About the first issue, I agree with @pygy , if resolve hook is called unconditionally on route, then the router should be flexible enough to match all the use cases.

And as Leo said, mithril provides a way to trash and re-create the view, using key on root component. Actually even the same page reload use could be done with the key arg normally. If you reload the current screen, then your data must have changed or there is no need to reload. Thus you should be able to get a new key from the new data.

Actually what is the default behavior of mithril now in v.1, when the route change and it is a different root component that is loaded? it seems that now mithril is using diff strategy all the time by default. Is it for the same reason that there is no more m.redraw.strategy api, that there is also no more bool param in m.render(domEl, vnode, true) to force the recreation of the view?

[isiahmeadows]

@lhorie

• "normal" route changes do what's currently the case for route changes: clear the tree and redraw everything from scratch.
• "diff" route changes (the proposed new kind) simply redraw by diffing, much like what you'd expect when you call m.mount to change an element's rendered component.

[lhorie]

Is it for the same reason that there is no more m.redraw.strategy api

Yeah, that's the primary reason. Basically, in some cases, people would have some expensive 3rd party thing somewhere in the layout (say a chart) that they wanted to maintain across route changes, and using m.redraw.strategy was confusing and changed the redraw semantics in a brittle way.

@isiahmeadows that doesn't seem like the right way to go, imho. It ties the choice of initialization semantics of a tree of components to the route change call, whereas imho the control of those semantics should be tied to either the tree of components or the route declaration

@veloce would having access to the previous path (as suggested by @pygy) solve your issue?

[barneycarroll]

The inconvenience borne from wanting the same behaviour in oninit and onbeforeupdate isn't exclusive to routes. The desirability of a onbeforeview which doesn't distinguish between first and subsequent executions applies equally to any component which needs to model convenient attrs input before the view executes. @lhorie really wants to lock down the current hyperscript / vnode API, so outside of routes, there's no prospect of a single hook for this. The methods available are variously:

// by direct reference
const onbeforeview = ( vnode, old ) => {
  // it's a redraw 
  if( old && someOtherCondition )
    // and we can do redraw-specific stuff
    return false

  // it's an initialisation
  vnode.state.viewHook = arbitraryLogic( vnode.attrs )
}

const Component = {
  oninit : onbeforeview,
  onbeforeupdate : onbeforeview
}

// or, point-free in external scope without external reference but with self-reference
const Component = {
  oninit : function(){
    Component.onbeforeupdate.apply( this, arguments )
  },
  onbeforeupdate : () => {}
}

// or point-free-execution, with no top scope reference, with run-time internal reference
const Component = {
  oninit : function( vnode ){
    vnode.tag.onbeforeupdate.apply( this, arguments )
  }
}

Moreover the leading render argument passed to resolve, and the naming of the render route method, feel awkward — and inconsistent when compared to the generic lifecycle methods and the various callback signal APIs used across Mithril (stream's return stream.HALT, onbeforeupdate's return false, onbeforeremove's trailing done callback, event handlers' native event mutation…).

It feels like all of these APIs were written by different people with different users in mind, which makes Mithril's API grammar really difficult to grok as a whole (I can imagine a lot of "aha, you missed this part of the docs" support resolutions). I appreciate that sounds like idle negativity: but I think it's important to strive for as consistent and generic an API as possible to allow powerful usage patterns to emerge.

@lhorie could you throw out some examples of how you would expect people to use the existing route APIs?

[isiahmeadows]

@lhorie I'd be okay with the other way, too. (I don't have much of a use case for multiple modes like that ATM, so I don't have a strong opinion either way.)

[isiahmeadows]

Would a special, non-URL character tag work? Something like this?

"/redraw/route": RedrawnComponent,
"@/diff/route": DiffRoute,

[veloce]

@lhorie I think the important part is that resolve is called unconditionnally on route change as @pygy said. For now it is called only for different paths, which breaks the "refresh page by hitting same route" fonctionnality.

I don't think that the previous path is useful since for a 'all' redraw strategy you can use the path as key, you don't need the previous one.

For the same behaviour as v0.2, ie. redraw 'all' on route (that means it should work also for page refresh), it seems the only way is to use a counter incremented in resolve as root component key. I think that's the solution I'll use to have a migration as smooth as possible from v0.2.

Now there is another problem I encountered: neither oninit/onremove nor resolve can replace what was the controller in v0.

The reason is that in v0 the controller.onunload was guaranteed to run before the new controller object was created on route change (I assume that empirically, I've never checked the code). This is no longer the case in v1: component's onremove is now async, and not called before the resolve. It means that if you need to execute logic on page exit that affects globally your app state (sometimes you need to do this), you can't use onremove. So to fix this, v1 needs a router hook onexit that would run on route change before the next resolve is called.

I understand that this last problem is not a bug, but from a migration point of view one must be aware of this subtle difference between controller in v0 and oninit/resolve in v1.

Should mithril have a router exit hook? Maybe it's not hard to code, then I'd say it's definitely useful.

[pygy]

@veloce in the absence of a onbeforeremove-triggered delay, onremove runs synchronously.

And onbeforeremove is sychronous, even though it only runs on the vnode that was removed from its parent (or the first vnode if you remove a fragment).

The current onbeforeremove implementation is a bit flaky (the way done is implemented allows the user to release Zalgo on the framework, and run the onremove phase twice), and it now I realize it conflates cleanup and delay concerns in ways that may not be desirable.

Would you want a global router exit hook, or per RouteResolver?

[veloce]

Ok I indeed use onbeforeremove for pages transitions. So the correct way to cleanup in case you want a synchronous behaviour is to use onbeforemove?

The router exit hook would have to be per RouteResolver for my use case.

[pygy]

@veloce the main issue is fixed now that #1290 has landed (but it hasn't been auto-closed because github only does that for the PRs merged into the main branch (next for Mithril)).

m.route.set(m.route.get()) now re-runs the onmatch hook.

1244 fixes the onbeforeremove flakiness.

You may want to close this and open another issue to discuss the onexit hook specifically if you still see a need for it.

[veloce]

Thanks @pygy
I moved since to a custom routing solution, I now use only m.render() because I want more control.
So I'm closing the issue since it's fixed now.

For now, in my use case, I've managed to do without the onexit hook. It is not that useful if we can rely on onremove or onbeforeremove; I think we have still an issue with those hooks: the fact onremove is called either synchronously or asynchrounously, depending on whether one uses onbeforemove is dangerous. I believe there is already a ticket for this.

[pygy]

@veloce there's #1245 that discusses it, feel free to chime in if you have anything to add.

[lhorie]

There's now support for keys and history state, which can be used in combination to reload a route. See http://mithril.js.org/route.html#key-parameter

[Niklas81]

I have routes where more than one attribute can change and should prompt oninit to reload, as oninit loads my data for specified page. I solved this automatic reload by having a main component:

var mainComponent = { oninit: function(vnode) {
                                vnode.state.get = vnode.attrs

                                /* 
                                More stuff here that loads data
                                */
                    },
                    onupdate : function(vnode) {
                        if (vnode.attrs !== vnode.state.get) { this.oninit(vnode) }
                    },
                    view : function(vnode){ /* insert view here*/ }
}

and a subcomponent with an onupdate method containing :

if (vnode.attrs.pageData !== vnode.state.pageData){this.oninit(vnode) }

Basically, the main component checks if the url has changed, and then reloads oninit, whereas the subcomponents check if the data it just received is different from it's current state. The subcomponents won't need to check if the url has changed, as that would prompt a reload of the main component, and thus the subcomponents are automatically reloaded as well.

My questions are: is this considered anti-pattern and if so, is there a better, recommended way of doing it? I am aware of the key attribute, but that only works if you have just one attribute that can change in the route, not if you have multiple.

[isiahmeadows]

@Niklas81 Could you file a new issue with that question?