m.route.setPath within lifecycle method breaks layout

[sebastiansandqvist]

Description:

Using the rewrite, I am setting up user authentication in a method that is very much the same as seen here:
https://github.com/tivac/anthracite/blob/master/src/lib/require-auth.js

Expected:

I expected m.route.setPath to route me to /login and erase the current page contents. It does not appear to matter which lifecycle method I put the routing logic in.

Actual:

m.route.setPath routed me to /login but left the current page contents intact, displaying both pages simultaneously.

I was able to fix this issue by wrapping m.route.setPath('/login') in a requestAnimationFrame. I think it makes sense that mithril should handle this logic on the next frame, but if the library itself can handle that implementation detail I think it makes more sense being handled there than in component code.

Edit:
I've created a minimal demo of this bug here using the rewrite:
https://jsfiddle.net/y8m56cg3/3/

There are interesting properties when combining this with a redraw:
https://jsfiddle.net/qs6b7qot/1/
https://jsfiddle.net/qs6b7qot/5/
https://jsfiddle.net/qs6b7qot/6/

And in some cases it can be fixed by wrapping m.route.setPath in a setTimeout or requestAnimationFrame.
https://jsfiddle.net/qs6b7qot/4/

This isn't always the case, but I haven't been able to reproduce the case where setTimeout doesn't solve the problem in a jsfiddle.

And here it is working properly with 0.2.0:
https://jsfiddle.net/8zybnxf6/1

[lhorie]

Fixed

[sebastiansandqvist]

In a few very rare cases I'm able to get the double layout to occur even after the fix.

Whenever it happens, it is immediately after m.route.set is called, and there is some state that changes almost immediately after the redraw. (For example, switching from a loading state to normal state within a few ms).

Will try to get a jsfiddle of it happening up this week, but it doesn't appear to be predictable. Sometimes doing one thing will cause the issue, and sometimes that same thing works just fine.

[sebastiansandqvist]

After a little bit of digging, I've found the solution but it breaks other tests.

In short, wrapping the contents of resolveRoute in anything that makes that code async fixes the bug in every case I have encountered (though I still can't get it to trigger in jsfiddle). https://github.com/lhorie/mithril.js/blob/rewrite/router/router.js#L75-L95

I've tried this solution with callAsync and with requestAnimationFrame and both worked just fine.

I was also a little bit confused to find that resolveRoute is being called on every redraw as well, not just when the route changes, but maybe that is intentional--I haven't looked too deep into the source yet.

Edit: instead of making everything async, it also works to just make the resolve part async
https://github.com/lhorie/mithril.js/blob/rewrite/router/router.js#L89

[lhorie]

Interesting. The fix I made was along those lines, but on this line: https://github.com/lhorie/mithril.js/blob/rewrite/router/router.js#L64

Perhaps it wasn't generic enough

[sebastiansandqvist]

I looked into this some more, and discovered that the bug occurs after I've called m.redraw() within an oninit function immediately and have routing setup. I wasn't literally doing this, but practically this is what was happening:

const myComponent = {
  oninit() {
    m.redraw();
  },
  view() {
    return m('div', 'foo');
  }
};

The reason this could occur is that I had a component which, in its oninit would map over an external stream to know when to redraw. If that stream already had a value, the redraw would end up being called immediately which would lead to the bug.

Here is the simpler version in action: https://jsfiddle.net/hwo1sctw/

It appears to only break when routing occurs. Replacing m.route with m.mount makes everything work: https://jsfiddle.net/hwo1sctw/1/

So, for the same reason that making resolveRoute async worked, using requestAnimationFrame(m.redraw) in the oninit instead also fixed the issue.
https://github.com/lhorie/mithril.js/blob/rewrite/router/router.js#L89

It makes sense that redrawing within oninit wouldn't work, but the outcome is so strange that I think this should be handled in some way. If the call to redraw would be ignored or deferred until the next tick whenever a redraw is in progress I think that would be a nice solution.

Edit: just realized this case is covered by the docs, too.
https://github.com/lhorie/mithril.js/blob/rewrite/docs/lifecycle-methods.md

[gilbert]

@sebastiansandqvist It looks like calling m.redraw from a lifecycle hook is recommended against, and documented as undefined behavior: https://github.com/lhorie/mithril.js/blob/rewrite/docs/lifecycle-methods.md#do-not-redraw-synchronously-from-lifecycle-hooks

[dead-claudia]

@lhorie When redrawing, what should m.redraw in the rewrite do?

1. Throw an error
   • It's already documented undefined behavior. We could easily make it defined behavior by simply throwing instead.
2. Queue a new redraw
   • This is pretty straightforward to implement. Most of the hacks in 0.2 (e.g. in mithril.exitable) that this would make impossible are already resolved with the new lifecycle hooks.
3. Abort the current redraw and start over completely, synchronously.
   • This is non-trivial to implement because half of the functions have to include rollback semantics. It could also break components by potentially leaving them in an invalid state.

[barneycarroll]

@sebastiansandqvist

The reason this could occur is that I had a component which, in its oninit would map over an external stream to know when to redraw. If that stream already had a value, the redraw would end up being called immediately which would lead to the bug.

Surely that's an application logic bug, and the initialisation code should know that an initial draw is forthcoming and therefore not trigger if the required data are already present?

The case against allowing redraws when a view execution is forthcoming (oninit, onbeforeupdate) is that whatever synchronous model changes occur will be addressed by the draw; any asynchronous model changes initialised or inferred in this context should introduce the redraw invocation in the appropriate callback.

Asking for a redraw after view execution — oncreate or onupdate — is a different matter: you might conceivably know by this stage that the preceding cycle is already stale. So in theory, there's an argument for respecting redraw invocations at any point after view execution. I would still be wary of allowing this, since I would treat any such usage as a code smell: if the lifecycle methods are used correctly, the model should not have changed over the course of the draw loop.

As @isiahmeadows says, the recursive redraws in Exitable were a hack to get around shortcomings in the lifecycle framework. If we encounter use cases that seem to legitimate further hackery in v1, they should be treated as indicative of shortcomings in lifecycle documentation or implementation. For that reason I would keep behaviour undefined, and deal with problem cases holistically as and when they arise.

[dead-claudia]

@barneycarroll What do you think of my ideas, though?

[barneycarroll]

Throw an error

I don't like this. Redraws can be spammed and since Mithril doesn't offer concrete tools or opinions for model and control flow architecture, situations like @sebastiansandqvist's — whereby a component registers redraw dependency on streams and immediately triggers — isn't wrong. Arguably there's nothing wrong with this and the silent noop is a feature. If we had a verbose debug build of Mithril you might make the case of throwing a warning, but it's definitely wrong for Mithril to presume an error here. If the developer notices unexpected behaviour, they can come and report it in their own words.

Queue a new redraw

This is what I was addressing above. A request to redraw before view execution is redundant; an explicit request to redraw within a view is a huge code smell and definitely shouldn't do anything IMO; a request to redraw immediately after DOM patching is either a code smell or an attempted hack that should be addressed holistically by the API.

Abort the current redraw and start over completely, synchronously

As you say: violates the current implementation and doesn't have a use case.

[dead-claudia]

@barneycarroll I agree that it's a terrible idea to redraw within a view or in most other lifecycle methods, especially synchronously. But with events, or just in case the state legitimately goes stale within a lifecycle method for some odd reason (possibly due to circumstances outside Mithril's control), I feel queuing (or more accurately, requesting a subsequent redraw) is probably the appropriate thing to do.

[barneycarroll]

But with events…

Could you elaborate on this point?

…or just in case the state legitimately goes stale within a lifecycle…

This isn't possible, since the lifecycle — once engaged by a draw — is sequentially synchronous.

[barneycarroll]

We should probably ditch the general lifecycle conversation here in favour of #1166 and reserve this thread for @sebastiansandqvist issue

[lhorie]

I'm moving this discussion to a new thread, since the original issue here has been addressed. See #1166