MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
14.02k stars 926 forks source link

First `m.redraw()` should be asynchronous #928

Closed veloce closed 8 years ago

veloce commented 8 years ago

The way it is coded right now any call to m.redraw() is synchronous, unless it is called subsequently within an animation frame.

That blocking behavior is counter intuitive as there is already a m.redraw(true) when we want to force synchronous redraw. So it would be logic that m.redraw() is always async and m.redraw(true) always synchronous.

And moreover it's not desirable in my opinion: all m.redraw() calls should always be async by default (and synced with browser for optimal rendering). Blocking calls can actually lead to some important lag when redraw is not used carefully and third party code also trigger (async) rendering. I've seen this in my application (lichess.org mobile app) where network driven events calling m.redraw() at very close intervals.

I tried to modify the code like this:

        try {
            //lastRedrawId is a positive number if a second redraw is requested before the next animation frame
            //lastRedrawID is null if it's the first redraw and not an event handler
            if (force) {
                redraw();
            }
            else if (!lastRedrawId) {
                lastRedrawId = $requestAnimationFrame(redraw, FRAME_BUDGET);
            }
        }

Like this the first redraw is asynchronous and subsequent redraws are debounced withing an animation frame. I tested that code in my app, it seems to work well. I won't open a PR because I don't understand all the logic in m.redraw() and I believe tests are affected by the fact the first redraw is synchronous. So this is more an issue for discussion :)

dead-claudia commented 8 years ago

IIRC they are unless they're forced. The problem is that there's no way of knowing when an async redraw is finished. That's the problem. (The correct fix would be to internally schedule a callback to run after the redraw completes.)

dead-claudia commented 8 years ago

Any behavior different is a bug.

dead-claudia commented 8 years ago

@lhorie Is my judgement of this correct?

pygy commented 8 years ago

The correct fix would be to internally schedule a callback to run after the redraw completes.

Hence #901

dead-claudia commented 8 years ago

@pygy Good point.

pygy commented 8 years ago

While m.hooks.next.route() was a solution for a problem I was having, the .redraw() version was more of a solution in search of a problem... until now :-)

—Pierre-Yves

On Mon, Jan 11, 2016 at 11:31 AM, Isiah Meadows notifications@github.com wrote:

@pygy https://github.com/pygy Good point.

— Reply to this email directly or view it on GitHub https://github.com/lhorie/mithril.js/issues/928#issuecomment-170501976.

dead-claudia commented 8 years ago

Closing due to lack of activity.

pygy commented 8 years ago

I had forgotten about this, and advocated for it independently in #1166.

The current behaviour is not predictable and is considered a bad practice...

The modified Promiz is also problematic in this regard.

dead-claudia commented 8 years ago

@pygy Let's go ahead and keep the discussion in #1166. Thanks! :smile:

pygy commented 8 years ago

Yeah, that message was mostly intended for @veloce who may not follow every issue.