What should m.redraw do when called from a lifecycle method

[lhorie]

Current behavior is undefined (and documented to be undefined). Typically, it overflows stack

@isiahmeadows' suggestions are to 1) throw, 2) rollback-and-restart or 3) request an async redraw

1) @barneycarroll has objections

2) @isiahmeadows points out this is highly non-trivial due to rollback semantics

3) this is similar to what React's setState does (see: http://thereignn.ghost.io/on-the-async-nature-of-setstate-in-react/ ). My concern is sometimes-async semantics is highly surprising and confusing

Other points to consider: allowing a mechanism for data to modifiable from a lifecycle method (which is by definition within the call graph of a render call) means that calls to render can no longer be guaranteed to keep state stable (think Angular's infdig).

I have been considering adding m.redraw.sync, which would make it easier to reason about the synchronicity semantics of the redraw irrespective of whether it's in a lifecycle method or not (but one would still need to be aware of the undefined semantics of synchronous redraws within lifecycle methods if they chose to use m.redraw.sync for whatever reason).

[barneycarroll]

In v0, a call to redraw during the draw lifecycle is a no-op. That makes perfect sense up till the point of DOM persistence — so IMO calls to redraw invoked during oninit, onbeforeupdate, onbeforeremove & view should follow this behaviour. As regards oncreate / onupdate, since these occur after Mithril has reconciled the internal vnode graph, I don't see why the call shouldn't be respected.

Worth pointing out that with the advent of stream-like props, we will likely see users create streams in component initialisers and potentially write to those streams or their derivatives downstream. Further derivatives may well set up conditions to redraw on stream update. In this scenerio one can imagine redraw being spammed during a view tree's initialisation, and all author intentions being met since the streams outcomes will have resolved before view execution. In this sense, silent redraw no-op during view composition becomes more of a feature in v1, as opposed to a code smell as it might have been in v0.

[dead-claudia]

I'd also like to point out that with 2 (rollback), there is also the risk of leaving user components in an invalid state, which is generally a bad idea.

[dead-claudia]

@lhorie With 3, I feel it may be a necessity to resort back to the async redraw semantics, the way this is going. It previously timed itself to use requestAnimationFrame unless called in a way that forces a sync redraw.

As for the API, I would strongly prefer a separate sync method, because then you can just do stream.map(m.redraw), knowing it's just going to run asynchronously regardless of what's in the stream, or m.redraw.sync(), knowing it's always synchronously.

[barneycarroll]

Yeah, can we just drop the rollback, draw during draw idea? We've stated again and again that it's an implementation nightmare, nobody has asked for it, and nobody can work out how or why anybody might want it. Would be fantastic if we could just not bring it up again unless there's new information.

@isiahmeadows the idea of a separate method for sync is a very good one. I support that proposal whole heartedly. Making redraw semantics more explicit can only be a good thing.

[dead-claudia]

@lhorie To clarify, my async-redraw idea would be to return a single-update stream. And it is entirely possible we could get away with not having a sync redraw, although the testing might become a little more complex.

@barneycarroll You could, in theory, disrupt a running redraw by running a sync one right in the middle of it, but Mithil could (and should IMHO) throw an error in that case to make it very clear it's not okay to do that. Async redraws won't have that problem, though.

[barneycarroll]

@isiahmeadows dunno, I think a no-op plus warning would do. Assume it's an incidental accident and move on, right? The case for redraw calls within a draw is setting up a prop redraw dependency, which could trigger upon initialisation. These redraw requests could well be synchronous, but I still wouldn't force the author to rewrite their init code to avoid immediate trigger, which I think we can still safely call a no-op — with a warning, in case the developer is actually expecting this scenario to do something — but the eagerness to presume author error and throw isn't justified IMO.

[dead-claudia]

If it's really intentional, you can always guard it with a try-catch and ignore all errors (I've done that before with other things). I'm suggesting throwing since it's likely to be an author error.

On Sat, Jul 23, 2016, 13:58 Barney Carroll notifications@github.com wrote:

@isiahmeadows https://github.com/isiahmeadows dunno, I think a no-op plus warning would do. Assume it's an incidental accident and move on, right? The case for redraw calls within a draw is setting up a prop redraw dependency, which could trigger upon initialisation. These redraw requests could well be synchronous, but I still wouldn't force the author to rewrite their init code to avoid immediate trigger, which I think we can still safely call a no-op — with a warning, in case the developer is actually expecting this scenario to do something — but the eagerness to presume author error and throw isn't justified IMO.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lhorie/mithril.js/issues/1166#issuecomment-234731508, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBKOEk7kVpVBY1e0v3OWvdGoAPkJCks5qYlZFgaJpZM4JQyIs .

[barneycarroll]

I don't know where this contention of likelihood is coming from, but I think the pattern I described is credible. I don't think people should be made to write try catches in application code to get around framework obnoxiousness, and I don't see what the benefits of error throwing are.

[dead-claudia]

What would be a use case of calling a synchronous redraw while another redraw is being run?

On Sat, Jul 23, 2016, 14:09 Barney Carroll notifications@github.com wrote:

I don't know where this contention of likelihood is coming from, but I think the pattern I described is credible. I don't think people should be made to write try catches in application code to get around framework obnoxiousness, and I don't see what the benefits of error throwing are.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lhorie/mithril.js/issues/1166#issuecomment-234732020, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBB_KhmVYoscm_ip_U4UcaO3G0Xpuks5qYli8gaJpZM4JQyIs .

[lhorie]

@isiahmeadows

var MyComp = {
  oncreate: function(vnode) {
    this.top = (document.body.offsetHeight - vnode.dom.offsetHeight) / 2
    m.redraw(true) //trying to avoid flickering
  },
  view: function(vnode) {
    return m("div", {style: {top: this.top + "px"}}, vnode.children)
  }
}

[dead-claudia]

@lhorie @barneycarroll

Okay. Here's my idea to try to mitigate the problem:

• m.redraw() - Schedule a requestAnimationFrame-guarded redraw that is always async.

  Returns a stream that is resolved with undefined and ended on completion, and the redraw request, if pending, can be aborted by ending the stream early.

• m.redraw.current() - Schedule a redraw in the current frame. If we're currently redrawing, then another redraw is requested. Otherwise, it starts redrawing immediately.

  Returns a stream that is resolved with undefined and ended on completion, and the redraw request, if pending, can be aborted by ending the stream early.

• m.redraw.redrawing() - Whether we are in the middle of a redraw.
• m.redraw.pending() - Whether any other redraws are pending in subsequent frames.
• m.redraw.pendingCurrent() - Whether any other redraws are pending in the current frame.

Yes, m.redraw.current() is sometimes async, sometimes not, but that's mostly unavoidable unless you're okay with scheduling a microtask (I'll note that a robust browser polyfill exists, but it's non-trivial).

[barneycarroll]

I don't think that level of complexity addresses any of the conceivable use cases we've described, it just makes the API surface harder to grapple with. I do think:

1. The separate m.redraw.sync method is a really good idea (that needs its own issue, since it's about API surface, not implementation).
2. Invoking a sync redraw during virtual DOM build (ie anywhere between the first oninit and the last view ) should produce an error, since it isn't possible or desirable to fulfil the author's explicit intention.
3. Invoking an async redraw during virtual DOM build should silently noop since it can safely be assumed that whatever state change triggered it has taken place and therefore will be persisted to the view.
4. Calls for sync or async redraw post-build (ie during oncreate, onupdate) should be allowed since it doesn't pose any implementation problems.

[pygy]

@barneycarroll re. implementation issues: currently, m.redraw()has as many render calls as there are mount points, run in the chronological mounting order. So a synchronous redraw called from a post-build hook in all but the last render would end up being sandwiched in between the render calls of the previous redraw, which may be surprising.

render_1(root_1)
    render_2(root_1)
    render_2(root_2)
render_1(root_2)

Edit: However, if you only want to redraw the current mount point, you can fetch the root.redraw function from vnode.dom by using

function nearestRedrawer(node) {
  while (!node.redraw) node = node.parentNode
  return node.redraw
}

(where root.redraw() is currently async and throttled).

[barneycarroll]

Here's a little tree with nested components, and couple of mountpoints with lifecycle logs. This kind of stuff becomes really hard to rationalise without reference :)

@pygy multi-tenancy is a rare enough scenario that this shouldn't be a common gotcha (and honestly, it's difficult to work out how that gotcha would manifest itself in terms of author expectation disparity).

What is the use case for an immediate post draw synchronous redraw? Let's not forget that requestAnimationFrame is a feature. @lhorie's example — inspecting DOM side-effects in order to inform the view model, which other parts of the current view depend on — is the only credible scenario for this I can imagine (I've been there). In this scenario, the author should be grateful for the internal requestAnimationFrame, rather than forcing the browser to recompute view despite its own stated inability to do so. If the author is afraid of flicker (we're talking about a situation where your implementation mandates 2 animation frames in order to achieve what would otherwise conceptually be a single frame), then we can assume — given they are adept and engaged enough to isolate relevant DOM props and accommodate this into their view model, and understanding the lifecycle loop well enough to put all this together — that they are adept and engaged enough to go the extra mile and do whatever necessary temporary display:hidden magic and extend their view model more comprehensively to cater for the fact that due to limitations in DOM, they are effectively dealing with a draw in 2 steps.

Like async components and forced redraw on route change in v0, I increasingly feel this is a nasty and dangerous API that legitimates bad practice. Just as forced redraw was there to allow people not to do proper teardown for 3rd party DOM integrations, and blocking XHR encouraged people to write views that would otherwise error, the raison d'être for sync redraw seems to be encouraging people to allow incomplete / semi-stale view models, with the dangerous get out of jail card that says it won't matter because you can immediately rerender. Far better would be not to offer people that temptation and let them acknowledge the underlying modelling / display issues, no? Shall we at least wait until somebody presents a use case born out of actual need, and just leave it out until then? It can always be patched in easily enough in 1.X if and when people really want it.

[dead-claudia]

@barneycarroll

What is the use case for an immediate post draw synchronous redraw?

I asked the same question, and here's the answer. It's not the obvious default behavior, so I intentionally proposed it to be its own method.

As for how it could be improved? I'd directly set vnode.dom.style.top instead of passing it through the vdom tree. It's easier and faster to manually handle that yourself. So IMHO that example isn't a good one.

// What it should be:
var MyComp = {
  oncreate: function(vnode) {
    vnode.dom.style.top = (document.body.offsetHeight - vnode.dom.offsetHeight) / 2
  },
  view: function(vnode) {
    return m("div", vnode.children)
  }
}

If you need to do things like that higher up a tree, you shouldn't be passing low-level details like that around at a high level. Instead, you should still be diffing and patching the relevant properties at a lower level in onupdate.

I agree that the use case isn't there anymore.

---

@lhorie @barneycarroll Here's my new suggestion:

m.redraw() schedules a redraw on the next requestAnimationFrame, and is always sync. It returns a stream that is resolved with undefined and ended on completion, and the redraw request, if pending, can be aborted by ending the stream early. If multiple requests are made within the same frame, it simply batches calling those into the next frame.

This should at least fix the problem. Also, note that m.render() will (and should) remain synchronous.

[dead-claudia]

Interesting (and possibly unrelated) side note: I believe you could almost literally create custom components that don't depend on Mithril's attribute diffing:

var div = {
  oncreate: function (vnode) {
    vnode.dom = document.createElement("div")
    // set initial attrs
  },
  onupdate: function (vnode) {
    // update attrs
  },
  view: function () {
    // here's the only catch...this won't work
    return []
  },
}

[pygy]

@isiahmeadows agreed with m.redraw() being always async, on the next frame. Otherwise we end up releasing Z̮̤͎̗̯̳ͬͭ̉ͮͭͅȁ̮̬͇̯̞̣̼̓l̲̭̫̗̝̉̄̍͒͑̇̽g̯̘̝ͤͨŏ̥.

Your stream-based idea also does make sense. I don't know why you'd want to cancel a pending redraw, but it would provide an onredraw hook too, which has been requested in the past.

Your diff-less scheme can be made to work. Return a single element in the view, replace it in oncreate and skip diff with onbeforeupdate(vnode, old) {return false} (where you can access old.dom).

[dead-claudia]

@pygy

BTW, if you close a stream, it's permanently closed.

But I agree such a hook would also be useful, but on a separate method. You could also use that for error handling: if an error occurs while rendering, you can emit an error event instead of just logging it to the console, leaving it uncaught.

Here's my current idea:

• var stream = m.redraw() - Redraws asynchronously and emits a single undefined when done, or the error if one occurred when rendering. Closes immediately afterwards. If a redraw request is already pending, this will piggyback off of that one.
• var stream = m.redraw.stream() - Returns a stream that emits undefined on each redraw.

[barneycarroll]

The redraw hook thing is a bit redundant because all idiomatic application code in Mithril is already structured into components, and all the code in components exists within lifecycle hooks — which are triggered by redraws. If you want to call a redraw and follow up with further actions when it resolves, you set a flag in component state and call your redraw; onbeforeupdate (or wherever suits), you sniff for said flag. The lifecycle hooks are the callbacks. So a canonical example looks like this:

const Square = {
  oncreate : vnode => {
    vnode.state.height = vnode.dom.clientWidth

    Promise.resolve().then( m.redraw )
  },

  view : vnode =>
    m( '.Square', {
      style : {
        display : 'inline-block',
        height  : vnode.state.height + 'px'
      }
    },
      vnode.children
    )
}

[dead-claudia]

@barneycarroll You may have a point. Here's an idea, how about a way to capture errors during lifecycle hooks, and observe them? That's very useful for things like error handling in production.

Here's my new two methods:

• var stream = m.redraw() - same as above: redraw, and emit once when done.
• var errorStream = m.redraw.error() - stream of errors thrown during component lifecycle events

[pygy]

var MyComp = {
  oncreate: function(vnode) {
    this.top = (document.body.offsetHeight - vnode.dom.offsetHeight) / 2
    m.redraw(true) //trying to avoid flickering
  },
  view: function(vnode) {
    return m("div", {style: {top: this.top + "px"}}, vnode.children)
  }
}

@lhorie That scenario could be solved without a redraw() by using a style vnode whose content is set dynamically during the oncreate phase.

var MyComp = {
  oncreate: function(vnode) {
    this.top = (document.body.offsetHeight - vnode.dom.offsetHeight) / 2
  },
  view: function(vnode) {
    return m("#MyComp", [
      m('style', {
        oncreate: function(styleVnode) {
          styleVnode.dom.styleSheet.cssText = "#MyComp {top: " + vnode.state.top + "px}"
        },
        onbeforeupdate: function(){return false}
      }, ""),
      vnode.children
    ])
  }
}

[pygy]

Also, even for single tenant apps, calling redraw(true) can cause onupdate to fire before oncreate.

m.mount(root, {view: function() {
  return [
    m("div", {oncreate: m.redraw.bind(null, true)}),
    m(".problem-here", {
      oncreate: console.log.bind(console, "oncreate"),
      onupdate: console.log.bind(console, "onupdate")
    })
  ]
}})

[pygy]

Globally, making render() atomic for a given root seems like the best course of action (by setting a flag on the root). Whether to throw or to noop is another story.

If a use case ever requires it, a synchronous post-redraw hook could be provided to schedule a subsequent sync redraw.

[lhorie]

I updated the docs to indicate m.redraw is to be considered asynchronous even if in some occasions they do run synchronously for perf reasons.

[dead-claudia]

@lhorie It'd be better if you document when it's synchronous and when it's asynchronous. If you're going to keep it sometimes-synchronous, that's definitely something that must be clearly noted, to prevent a whole host of confusion later on.