Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should React use requestAnimationFrame by default? #11171

Closed
dakom opened this issue Oct 10, 2017 · 37 comments
Closed

Should React use requestAnimationFrame by default? #11171

dakom opened this issue Oct 10, 2017 · 37 comments

Comments

@dakom
Copy link

@dakom dakom commented Oct 10, 2017

Consider the following sample code: (pasted here too)

class extends React.Component {
    private canRender: boolean = false;
    private latestData: any;

    constructor(props) {
        super(props);

        let nJobs = 0;
        let lastRenderTime: number;
        props.someObservableThing.listen(data => {
            nJobs++;

            this.latestData = data;

            if (this.canRender) {
                const now = performance.now();
                this.canRender = false;
                this.setState({
                    data: this.latestData,
                    jobsPerRender: nJobs,
                    fps: (lastRenderTime === undefined) ? 0 : 1000 / (now - lastRenderTime)
                });
                nJobs = 0;
                lastRenderTime = now;
            }
        });

        this.state = {};
    }

    /* Lifecycle */
    componentDidMount() {
        this.canRender = true;
    }

    componentDidUpdate() {
        this.canRender = true;
    }

    render() {
        outputStats(this.state);
        return this.state.data === undefined ? null : <View {...this.state.data} />
    }
}

When outputStats is hit - I'm getting framerates of like 2000fps. In other words requestAnimationFrame does not seem to be a limiter for react itself.

Is this correct?

(as a slightly separate topic- if that is true, for animation things do you think it would be good to simply wrap the if (this.canRender) {} block in a requestAnimationFrame()? I guess that's not really a React question though since the observableThing could also be capped via ticks...)

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Oct 10, 2017

No, React does not currently use rAF at all. Although in the future we plan to use it to polyfill requestIdleCallback.

if that is true, for animation things do you think it would be good to simply wrap the if (this.canRender) {} block in a requestAnimationFrame()?

Sounds good, but don’t forget not to schedule another frame if you’ve already scheduled one.

@jakearchibald

This comment has been minimized.

Copy link

@jakearchibald jakearchibald commented Jan 15, 2018

Following on from a conversation on twitter. Rough story so far:

Me:

I naively thought React's render-related callbacks & DOM updating would be debounced using requestAnimationFrame, but that doesn't appear to be the case.

Is there a way to opt into that, or am I missing something?

@gaearon:

Not currently. We do batching between different setState()s within one event handler (everything is flushed when you exit it). For many cases this works well enough and doesn’t have pitfalls of using rAF for every update.

We are also looking at asynchronous rendering by default. But rAF doesn’t really help much if the rendered tree is large. Instead we want to split non-critical updates in chunks using rIC until they’re ready to be flushed.

Me:

Could result in a lot of extra work if you're updating on mousemove, or a couple of things land in the same frame, eg a network response + a mouse click.

What pitfalls are you seeing with rAF?

@gaearon:

Yes, we use a concept of “expiration”. Updates coming from interactive events get very short expiration time (must flush soon), network events get more time (can wait). Based on that we decide what to flush and what to time-slice.
As for rAF, it doesn’t play very well with browser stuff like inputs. We need to flush some updates immediately during the event. At that point we’re back to talking about priorities.

Me:

Which updates needed to flush immediately? FWIW Chrome & Firefox have moved some input events to the render steps, eg mousemove. Some are already there, eg touchmove, scroll, resize, intersection/resize observers

@gaearon

For cases when you really need it you can always use rAF yourself and flush during its callback. That is only beneficial in some scenarios (animation) because it’s exact same amount of blocking work as doing an update synchronously. So it doesn’t help that much, and has pitfalls.

Controlled inputs need to flush synchronously, else the value gets out of sync.

It's true that you can do this sort of stuff yourself, but not totally easy to manage, especially across components. Some examples:

If you have a component updating on an interval (like the Clock), but it also runs a decorative animation using requestAnimationFrame, you'll end up with two DOM updates for a single frame, one for the interval, and one for the rAF. This includes render(), diffing, and an actual DOM update.

If you're updating a component based on a mousemove listener, this can mean 20+ updates per frame (eg Edge/Safari with a wired mouse). If you call requestAnimationFrame within these mouse move events, you'll still get 20 DOM updates. You'd need to make sure you were scheduling one rAF per frame. Not too hard I guess.

If you have a component running an animation, containing a component also running an animation, the inner component's DOM gets modified twice per frame. If you have another animating component within that one, it gets modified three times per frame, and so on. Demo. I suppose shouldComponentUpdate would reduce some of the computation here, but it'd be nice if it weren't an issue. It feels like React should be updating on rAF, and it'd know that all three components have pending updates, so run a single update starting at the parentmost element that needs updating.

I'm really excited about the rIC stuff coming to react, but I thought the above pitfalls were already solved. As in, "Update state in tasks, do your rendering in render(), React will ensure you aren't doing more work than necessary."

Keen to hear more on what makes this tricky! Especially if it's something we can find a spec solution for.

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jan 15, 2018

Reopening for discussion

@gaearon gaearon reopened this Jan 15, 2018
@gaearon gaearon changed the title requestAnimationFrame not in react-core? Should React use requestAnimationFrame by default? Jan 15, 2018
@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jan 15, 2018

Today is a US holiday but I asked @acdlite to bring me up to date on his thinking about this. We actually had a rAF schedule priority earlier in the async implementation but later cut it. I don’t fully remember what happened there and will reply after I learn more about it. If I forget please ping me, there’s a lot of stuff going on and I can accidentally miss it in an old issue.

@jakearchibald

This comment has been minimized.

Copy link

@jakearchibald jakearchibald commented Jan 15, 2018

@gaearon suggested that controlled inputs could be the issue. Eg:

  1. User types in text input.
  2. Task is queued to fire the 'update' event, in which:
    1. The component's state is updated to reflect new input.

Given that the browser may render between tasks, it's possible that render() will happen before the task executes, meaning the old state is reapplied to the text input.

But, unless I'm missing something, it feels like this could happen without rAF. If the input's value is changed, and a task is queued to fire update, it's possible that there's an item earlier in the queue that will update state, trigger rendering, and create the same issue.

I couldn't recreate the issue by flooding the task queue, which makes me think that the html spec isn't quite right. It's more likely that a single task is queued to update the input's value and fire the update event. But if that's the case, rAF shouldn't be a problem either.

@RByers can you shine some light on browser behaviour here? Is it possible for the user to type in a text input, and rAF to happen (and change the input's .value) before the update event is dispatched?

@jquense

This comment has been minimized.

Copy link
Collaborator

@jquense jquense commented Jan 15, 2018

I can't speak for scheduling but i a am fairly familiar with the limitations around input value setting. The main one with updates over an async boundary is that cursor position will reset if the update isn't synchronous, making typing impossible. Since react is forcibly keeping the value to the prop value (vs the typed value) an input's value is "reset" to the synced value on each update until/unless the user updates the prop value to match what onInput reported the value should be

@jakearchibald

This comment has been minimized.

Copy link

@jakearchibald jakearchibald commented Jan 15, 2018

@jquense I'm having trouble recreating that. What am I missing? https://static-misc.glitch.me/input-test.html

@syranide

This comment has been minimized.

Copy link
Contributor

@syranide syranide commented Jan 15, 2018

@gaearon I'm working on a totally separate non-React project where I've been trialing using requestAnimationFrame for the same purpose. But I've found out that Edge seems to schedule the callback very poorly and there's a very noticeable delay (dragging stuff with cursor) vs not using requestAnimationFrame.

I don't think I've figured it all out yet, but one part of the problem is that the callback seems to be called immediately after vsync and before any other events have occurred, which means that you will effectively be rendering with the mouse data of the previous frame (mouse events are also triggered many times within the same frame). On my computer mouse move is triggered 4 times per frame which means it should only add a delay of about 4ms, but it seems noticeably worse. I would estimate that it doubles the visual latency.

This should probably apply to clicking/typing/etc too. Maybe you can pressure Edge into fixing it or the trade-off is worth it, but it doesn't live up to the promise of being the same but better it seems.

EDIT: Being scheduled after vsync with events being emitted after it should obviously incur a more than 1-frame delay over just rendering out the changes immediately, but Chrome still feels faster. Maybe there's another 1-frame delay somehow caused by Edge as well that pushes it to feel more sluggish than Chrome.

@jakearchibald

This comment has been minimized.

Copy link

@jakearchibald jakearchibald commented Jan 15, 2018

@syranide I think Safari has the same scheduling bug. I'll file one for edge too.

WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=177484
Edge bug: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/15469349/

@syranide

This comment has been minimized.

Copy link
Contributor

@syranide syranide commented Jan 15, 2018

@jakearchibald Nice! I don't think the description for the Edge-issue is entirely correct though, but I'm not sure either. Scheduling after vsync should be correct, it's just that all other events should be emitted before it, but Edge seems to emit events as they come rather than only at the beginning of a frame. Which means that your click-event is received a few milliseconds after the animation callback has been executed and before the next paint.

@jakearchibald

This comment has been minimized.

Copy link

@jakearchibald jakearchibald commented Jan 15, 2018

@syranide requestAnimationFrame callbacks should be called within the render steps (which are vsync'd), but before style calculation and paint.

https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model - rAF callbacks are in step 7.10, style calculation happens as part of 7.11, paint happens as part of 7.12.

@syranide

This comment has been minimized.

Copy link
Contributor

@syranide syranide commented Jan 15, 2018

@jakearchibald Ah, I guess we're saying the same thing in the end. Ultimately it seems the current event model of emitting events as they come in Edge is incompatible with the spec for requestAnimationFrame, they could fix it without changing the event model but then they would incur a 1-frame delay for everything (or end up rerendering multiple times per frame). But IIRC correctly Edge already plays badly with requestAnimationFrame and caps it at 60fps, so yeah...

@dakom

This comment has been minimized.

Copy link
Author

@dakom dakom commented Jan 15, 2018

Sorry if I'm a bit out of the loop - but is there a problem doing something like (pseudo-ish code):

onMouseMove = evt => setState({lastMove: evt});
onTick = evt => setState(applyMovement(this.state.lastMove, targetObject));

i.e. store the simple values on move, and do the heavy lifting only on rAF? Is there something in React itself that prohibits this solution on the app side (e.g. maybe setState will miss the window)?

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jan 15, 2018

Copy-pasting @acdlite’s response to me over messenger:

My understanding is, Jake is suggesting that the default for non-input events should be animation priority, so users don't have to think about opting-into that mode. Which is probably better than today, where the default is sync.

  1. That would be a breaking change, for a subset of the reasons async is a breaking change (wouldn't get interruptions, but would still have stale this.state issue). (Dan: this means it would have to happen no earlier than 17.0, just like we can’t enable async mode in 16.x yet)

  2. We think the default priority for non-input events should be low priority (Dan: we call "flush within a second or two" the low priority). In that model, you have to opt into animation mode, anyway. So you might as well call requestAnimationFrame yourself and use flushSync (Dan: it's a new API that forces a synchronous update).

Also if you're integrating with another library, it may already have its own requestAnimationFrame. You don't want to call rAF again from inside that one, because then React won't update until the next frame. So at that point, flushSync is what you want, anyway.

As far as requests for the browser vendors, I thought it would be cool if there were some way to know when an event was about to be fired on a target. That way we could flush pending interactive updates to the DOM before the browser decides how to interpret the event. Right now, the best we can do is wrap the user-provided event handler, but by the time our wrapped handler is called, it's too late.

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jan 15, 2018

(I think I have enough context now to answer more questions so keep them rolling)

@sebmarkbage

This comment has been minimized.

Copy link
Member

@sebmarkbage sebmarkbage commented Jan 15, 2018

Mouse moves are fairly unique in that it is almost always safe to drop an event (at least if you also drop the corresponding mouse over/out). You can’t tell if one was dropped.

Most events are intentional and require permanent effects such as two clicks happening the same event loop. You need the result of the first to happen before the second one.

Since these events can happen multiple times in a single frame, rAF batching is not really a suitable fix to that problem.

When the same effect happens with an OS driven sequence it has to be synchronous. Such as text input.

Many effects can be deferred until later than next frame and that’s why are main strategy for that will be requestIdleCallback.

For things that are dependent on a per frame level, such as mouse moves, animations, scrolling and touch handling, it is often important that they’re coordinated so that they happen all at once. It is also safe to drop extra events if there are any. It is also often necessary to update even if there are no other events than time. E.g. for inertial scrolling.

For those we’ll have a special strategy. As mentioned before, a tricky problem is to set up the coordinators. If two parts of an app or libraries set up their own rAF it is difficult to determine which one will happen first so letting the user schedule one rAF and React schedule another to flush the work set up in one will be difficult to guarantee that they’re in the same frame. It can be [flush, setState] or [setState, flush].

It is more likely that for these a better model will not actually be to set state but to “react” to changes in global states like mouse position and time. The state is only when that cause some permanent change such as change in inertia, or application state but that doesn’t follow the same rules as per-frame rules in that they can’t be dropped.

@jakearchibald

This comment has been minimized.

Copy link

@jakearchibald jakearchibald commented Jan 16, 2018

@gaearon

My understanding is, Jake is suggesting that the default for non-input events should be animation priority, so users don't have to think about opting-into that mode.

Hmm, not quite. I'm not talking about debouncing event handling, just the render steps. The idea is that you'd call setState as frequently as you want, but the resulting render steps would be debounced by requestAnimationFrame, as there's no point rendering more frequently than the display is capable of displaying.

Debouncing events might also be useful, and it's something browsers are starting to do more of, but I think it's a separate thing. Also tricky when it comes to preventDefault etc.

As far as requests for the browser vendors, I thought it would be cool if there were some way to know when an event was about to be fired on a target. That way we could flush pending interactive updates to the DOM before the browser decides how to interpret the event. Right now, the best we can do is wrap the user-provided event handler, but by the time our wrapped handler is called, it's too late.

Something to flush the UI task queue sounds doable, but I don't really understand the problem yet, so I'm not sure if it's the right fix for the problem. Right now, it seems that React doesn't debounce rendering to rAF due to one of the following:

  1. Design choices in React.
  2. Browser bugs with rAF.
  3. Missing/insufficient standards.

The stuff around input updating makes it feel like the issue could be 2 or 3, but I haven't been able to recreate the issue, or think up a mechanism that'd cause the issue. Is there a recreation of the issue anywhere? Either a reduction, or a version of React that has the problem? I'd like to poke around it and get to grips with the issue.

@sebmarkbage

Most events are intentional and require permanent effects such as two clicks happening the same event loop. You need the result of the first to happen before the second one.

Since these events can happen multiple times in a single frame, rAF batching is not really a suitable fix to that problem.

I don't quite follow, unless you're talking about debouncing events, which isn't what I mean. If a network response landed to update data for the current view, and a click was received to change the view, and both happened before the next render, React currently updates the DOM twice, once to update the current view (for the network request), and again to change the view (for the click). It feels like rendering should be debounced to requestAnimationFrame, so setState is called twice, but the result is only rendered once, since the user is only going to be able to see one outcome anyway.

My assumption here is that setState is much cheaper than rendering (including the actual DOM update).

By 'rendering' I mean a component's render function, and the various callbacks that are related to rendering, such as shouldComponentUpdate.

For things that are dependent on a per frame level, such as mouse moves, animations, scrolling and touch handling, it is often important that they’re coordinated so that they happen all at once.

Unless I'm missing something, the model I'm thinking of would give you that for free. In these various events you'd update state, cheaply, perhaps 10 times a frame, but the resulting render would happen once a frame, and your state would be a result of changes made during those 10 events.

If two parts of an app or libraries set up their own rAF it is difficult to determine which one will happen first so letting the user schedule one rAF and React schedule another to flush the work set up in one will be difficult to guarantee that they’re in the same frame. It can be [flush, setState] or [setState, flush].

That's true. I wonder how much of an issue this would be in practice. If so, React could provide its own rAF, so it can flush after all queued callbacks.

@jakearchibald

This comment has been minimized.

Copy link

@jakearchibald jakearchibald commented Jan 16, 2018

I've tried to create a reduction of an input that sets state on 'input' and updates on requestAnimationFrame. I'm not seeing anything go missing, but maybe there's a particular browser where it happens?

https://event-loop-tests.glitch.me/raf-debouncing-input.html

@dakom

This comment has been minimized.

Copy link
Author

@dakom dakom commented Jan 16, 2018

@jakearchibald

In these various events you'd update state, cheaply, perhaps 10 times a frame, but the resulting render would happen once a frame, and your state would be a result of changes made during those 10 events.

A possible (albeit unlikely) scenario to consider might be tracing a path with the mouse. If we imagine that the entire motion took place between frames, a debounce would only have the final position rather than the whole path since setState wouldn't inherently accumulate the updates (and if it did accumulate - then it would be wasteful in most other scenarios... and if I'm not mistaken it ends up losing the optimization anyway since you'd want to process all those events before the next tick).

I'm not saying this is likely or something React needs to concern itself with, but it is a possible gotcha

The idea is that you'd call setState as frequently as you want, but the resulting render steps would be debounced by requestAnimationFrame, as there's no point rendering more frequently than the display is capable of displaying.

Another edge case to consider would be where the render target is audio, hardware, raw data - or something other than visual ticks. Both in terms of missed events and specifying a limiter other than rAF

Both of those points are way outside the norm of React usage and it can't be all things for all people, but felt it's worthwhile to think these things out loud :)

@jakearchibald

This comment has been minimized.

Copy link

@jakearchibald jakearchibald commented Jan 16, 2018

@dakom

A possible (albeit unlikely) scenario to consider might be tracing a path with the mouse. If we imagine that the entire motion took place between frames…

I think this is unrelated. Chrome/Firefox already debounce 'mousemove' to the render steps. Chrome exposes the full series of events using getCoalescedEvents for the cases where you need the full frequency of mouse positions. This is better than the task-based model Edge/Safari uses, as Chrome can still deliver the full series of event objects even if the main thread was busy. I've built a demo if you want to experiment with this kind of event timing.

But even then, wouldn't you need to be maintaining an array of points so you don't lose this data on the next render()? Unless I'm missing something, the idea of React is being able to describe the content of a component, from scratch, based on props + state.

Another edge case to consider would be where the render target is audio

If you need realtime audio processing, it really needs to be off the main thread, eg audio worklets.

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jan 16, 2018

My understanding is, Jake is suggesting that the default for non-input events should be animation priority, so users don't have to think about opting-into that mode.

Hmm, not quite. I'm not talking about debouncing event handling, just the render steps. The idea is that you'd call setState as frequently as you want, but the resulting render steps would be debounced by requestAnimationFrame, as there's no point rendering more frequently than the display is capable of displaying.

I think you meant the same thing. Andrew was being too concise, but by “default for non-input events” he means “the priority under which setStates that originate inside non-input events will be flushed”.

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jan 16, 2018

Most events are intentional and require permanent effects such as two clicks happening the same event loop. You need the result of the first to happen before the second one.

Since these events can happen multiple times in a single frame, rAF batching is not really a suitable fix to that problem.

I don't quite follow, unless you're talking about debouncing events, which isn't what I mean.

I don’t think that’s what Sebastian is saying. I think what he’s saying is that it’s important for the result of every click event to flush before the next click event. Consider a button that sets disabled={this.state.hasSent} and onClick={this.state.hasSent ? null : this.sendMoney}. If sendMoney sets hasSent to true, the product developer expectation is that onClick will not fire again. Otherwise two fast clicks can send the money twice, and that is a very error-prone conceptual model.

This is why we think it is important to treat setState inside interactive events (such as clicks) differently from setState inside, for example, network responses. And my understanding is that our plan is to flush “interactive” updates (like clicks) at the end of the event, but flush “other” updates (including network) with a low priority, chunked over rIC.

Andrew’s earlier point was that once we switch over to that approach, using setState for animation will not be practical by default because React will only flush it once a second or so. To fix this, the user would need to move their setState calls into a special call that flushes immediately (called flushSync). This “opts in” a setState into being processed synchronously. Since it is important when this happens we think it would be user’s responsibility to put that call inside a rAF. However we don’t exclude providing some hook to coordinate this with other libraries.

To sum up:

  1. We think delaying all flushing until a rAF breaks React rendering model and developer expectations in cases where the value of event handler depends on the state, or when event handlers read the state.
  2. We plan to keep setState inside of interactive events (such as click) acting like now, with flushing at browser event boundary.
  3. Nevertheless we plan to “downgrade” any other setState calls to low priority, meaning that if it comes from a network request or some non-interactive event, it will be processed separately, split over rIC and flushed within a second or two.
  4. We will offer a new flushSync(fn) method as an opt out of async-by-default behavior. If setState is called inside of fn it is flushed right after it exits. This is an important escape hatch for some scenarios where you really need to manually flush the update before React considers it necessary. For example if you’re in a click handler and you want to measure a DOM node after a setState and use that information for final rendering result.
  5. Making setState low-priority outside of events we decided must flush (like clicks) will “break” users who use it for animation (because their updates won’t be flushed inside events like onMouseMove—the exact scenario you’re not happy with). As a migration path we will ask them to call their setState inside flushState in a requestAnimationFrame callback. We don’t want to “own” that callback because we’re not the only ones who might want to use it, and therefore the user should be in control of it.

Does this help at all? I might have made a mistake somewhere so I’ll ask Andrew and Seb to verify, but this is my understanding.

The fifth point is not a final decision, we just haven’t gotten to it yet. We might use a strategy that’s a bit more obvious to the user or less error-prone. The intended takeaway is that: (1) I don’t see how we could use rAFs for clicks, (2) we already have a more efficient strategy in the work for non-clicks (rIC) so adding rAF in the mix doesn’t make it better, (3) we’ll need to think more about animation-specific use case, but it will likely build on top of the more flexible flushSync promitive we’ll have to expose anyway, (4) the user won’t get over-rendering animation by default as you’re worried, instead the animation will flush too rarely, leading them to research the right rAF solution (which we will document or provide a convenience API for).

@jakearchibald

This comment has been minimized.

Copy link

@jakearchibald jakearchibald commented Jan 16, 2018

@gaearon

Consider a button that sets disabled={this.state.hasSent} and onClick={this.state.hasSent ? null : this.sendMoney}. If sendMoney sets hasSent to true, the product developer expectation is that onClick will not fire again. Otherwise it is a very error-prone conceptual model.

Ahhhhh yes, that makes total sense.

my understanding is that our plan is to flush “interactive” updates at the end of the event, but flush “other” updates (including network) with a low priority, chunked over rIC.

Yeah, given the above, this model makes sense, as does the rest of your summary.

Making setState low-priority outside of events we decided must flush (like clicks) will “break” users who use it for animation (because their updates won’t be flushed inside events—the exact scenario you’re not happy with). As a migration path we will ask them to call their setState inside flushState in a requestAnimationFrame callback. We don’t want to “own” that callback because we’re not the only ones who might want to use it, and therefore the user should be in control of it.

I guess it'll also break things if anyone's already using rAF to debounce, as they'll be throttled to rIC instead. I guess this would break the clock demo too, as the timing is important.

There's still the case of nested components with animations, but maybe the answer is "Don't pipe your animations through React", which seems sensible to me, but folks have tried to convince me otherwise in the past. Or, as you say, provide some specific opt-in to schedule a render debounced by rAF, but in a React-aware way, so parent & child only render once.

Thank you for going through the reasoning & the future scheduling of React!

@jquense

This comment has been minimized.

Copy link
Collaborator

@jquense jquense commented Jan 16, 2018

the product developer expectation is that onClick will not fire again.

The interesting thing to me is that (at least in v15) this exact situation does happen. I mention it because as a react developer my expectation is already that I can't trust React to flush state changes fast enough to prevent double clicks

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jan 16, 2018

The interesting thing to me is that (at least in v15) this exact situation does happen.

If that happens, it sounds like a bug to me, rather than an intentional decision. I haven't seen that but if you can reproduce please file an issue and we'll look.

The state changes enqueued inside event handlers should always be flushed on exiting the event handlers both in React 15 and 16.

I can imagine that React 15 could have some sort of issues with multiple roots and edge cases like renderSubtreeIntoContainer. React 16 solves that by completely forbidding re-entrance in the algorithm (and adding portals). So if there was a bug, I'd assume it's solved.

@jakearchibald

This comment has been minimized.

Copy link

@jakearchibald jakearchibald commented Jan 16, 2018

@Cryrivers that won't be affected. The event handlers aren't being deferred.

@RByers

This comment has been minimized.

Copy link

@RByers RByers commented Jan 16, 2018

I haven't read the whole thread, so apologies if I miss something. Replying to:

@RByers can you shine some light on browser behaviour here? Is it possible for the user to type in a text input, and rAF to happen (and change the input's .value) before the update event is dispatched?

By "update event" I assume you mean "change", right? And by "rAF" to happen, you mean the rAF callback is invoked? If so, then yes I believe it is possible as follows:

  • Main thread could be busy processing some task
  • Input task arrives and is queued
  • rAF task arrives and is queued
  • When input task is processed it queues a task to dispatch change
  • Before that task gets a chance to run, the previously queued rAF task runs and the rAF callback sees the updated value of the input before the change event has been dispatched.

/cc @dtapuska, who knows more about this than I do.

@jakearchibald

This comment has been minimized.

Copy link

@jakearchibald jakearchibald commented Jan 16, 2018

@RByers My bad, I mean "input", or "keydown" rather than "change". I couldn't recreate the issue here https://event-loop-tests.glitch.me/raf-debouncing-input.html, but that doesn't mean it isn't possible.

@NE-SmallTown

This comment has been minimized.

Copy link
Contributor

@NE-SmallTown NE-SmallTown commented Jan 17, 2018

@gaearon

We will offer a new flushSync(fn) method as an opt out of async-by-default behavior. If setState is called inside of fn it is flushed right after it exits. This is an important escape hatch for some scenarios where you really need to manually flush the update before React considers it necessary. For example if you’re in a click handler and you want to measure a DOM node after a setState and use that information for final rendering result.

Could we use setTimeout to simulate the work which flushSync do, i.e.

class App extends React.Component { 
  constructor(props) {
    super(props)
    
    this.state = {}
  }
  
  componentDidMount() {
    setTimeout(() => {
      this.setState({barValue: 3})
      console.log(`syncSetState: ${this.state.barValue} ${document.querySelector('#foo').getAttribute('bar')}`)
    }, 2000)
  }
  
  render() {
     console.log(`render ${this.state.barValue}`)

     return (
    	<div>
    	  <h1 id='foo' bar={this.state.barValue}>Hello, {this.state.barValue}</h1>
    	</div>
      );
  }
}

ReactDOM.render(<App />, document.getElementById('root'));

It will output:

render undefined
render 3
syncSetState: 3 "3"
@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jan 17, 2018

No because:

  • It's not going to be synchronous because you always have to wait for the timeout (whereas flushSync needs to be)
  • In future versions we plan to make setState in a timeout async by default
@dakom

This comment has been minimized.

Copy link
Author

@dakom dakom commented Jan 17, 2018

By default for right now - is the path from setState to componentDidUpdate synchronous (even if only sometimes)?

Specifically - if I have some code in cDU that must execute in a different stack than where setState was triggered, must I push it off manually? (e.g. via setTimeout)

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jan 17, 2018

It depends on where you call setState.

  • In React 16, if you call setState inside a React event handler, it is flushed when React exits the browser event handler. So it's not synchronous but happens in the same top-level stack.

  • In React 16, if you call setState outside a React event handler, it is flushed immediately.

@johnywith1n

This comment has been minimized.

Copy link

@johnywith1n johnywith1n commented Aug 3, 2018

Consider a button that sets disabled={this.state.hasSent} and onClick={this.state.hasSent ? null : this.sendMoney}. If sendMoney sets hasSent to true, the product developer expectation is that onClick will not fire again. Otherwise two fast clicks can send the money twice, and that is a very error-prone conceptual model.

In this example, you've set both the disabled attribute and checked that flag in the onClick handler. Does this mean that it's not sufficient to only set the disabled attribute and that you also need to check inside the click handler to prevent these kinds of click more than once issues?

@gaearon gaearon closed this Aug 30, 2018
@dakom

This comment has been minimized.

Copy link
Author

@dakom dakom commented Aug 30, 2018

Thanks @gaearon and everyone - I learned a lot from following this thread!

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Aug 30, 2018

FWIW we've since stopped using requestIdleCallback because it's not as aggressive as we need. Instead we use a polyfill that's internally implemented on top of requestAnimationFrame. Although conceptually we still use it to do idle work.

@augbog

This comment has been minimized.

Copy link

@augbog augbog commented Oct 8, 2018

^ Was curious about that and for others that are curious as well, I believe this is the polyfill code which now lives in src/packages/Scheduler.js (feel free to correct me if I'm wrong though). Thanks for calling that out :)

@kushalmahajan

This comment has been minimized.

Copy link

@kushalmahajan kushalmahajan commented Nov 7, 2018

@gaearon

We will offer a new flushSync(fn) method as an opt out of async-by-default behavior. If setState is called inside of fn it is flushed right after it exits. This is an important escape hatch for some scenarios where you really need to manually flush the update before React considers it necessary. For example if you’re in a click handler and you want to measure a DOM node after a setState and use that information for final rendering result.

Is this now possible? Coz, I remember I came across this in last few months and I've to bring up React docs to tale help of componentDidUpdate() to have the measurement of DOM nodes. Please if you can provide a link to where can I find the updated way to do something like

this.setState({ height: response.value });
const height = this.state.height; // updated height

I try to follow major updates around in JS community but sorry if I've missed out in my heavy work schedule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.