-
Notifications
You must be signed in to change notification settings - Fork 45.6k
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
useEffect
can be synchronous?
#22506
Comments
I think, it is related to that story: #21400 |
#21400 looks related. However, in both of the cases in this issue, there is no |
It is important for effects code to be resilient to being run async or sync. (If you need to introduce async code for some reason, you could use a timeout– just be sure to return a cleanup function that calls
Yes, as is mentioned here: If something (usually a layout effect) schedules sync work, React has to flush any pending passive effects before processing the new work– so the overall order will be preserved. |
cc @rachelnabors and @gaearon for making a note of this in the effects docs |
Hi @bvaughn, Thank you for your prompt reply. Based on my reading of the I think this would be great knowledge to surface in the documentation just so that people know about this behaviour 👍 |
Here is a I'm still hashing it out, writing tests etc. import { DependencyList, useEffect } from 'react';
type CleanupFn = () => void;
// Under certain conditions, effects queued with `useEffect` can be flushed *synchronously*
// See https://github.com/facebook/react/issues/22506
// For most effects it is fine to be run either sync or async
// However, some effects do need to be in a task after rendering
export default function useDeferredEffect(
effect: () => void | CleanupFn,
deps?: DependencyList,
) {
useEffect(
function setup() {
let cleanupFn: CleanupFn | null = null;
const timeoutId = setTimeout(function task() {
cleanupFn = effect() ?? null;
});
cleanupFn = () => clearTimeout(timeoutId);
return function cleanup() {
cleanupFn?.();
};
},
// Ignoring 'effect' from the dependency array
// eslint-disable-next-line react-hooks/exhaustive-deps
deps,
);
} relevant eslint configuration:
Thinking through:
|
I guess ideally you wouldn't have to wait - but maybe it doesn't matter. Maybe it's worth classifying for what kind of effects you'd like to use this
What I'm slightly afraid of - and why I'm proposing to list the kind of effects for which this could matter. If the thing you are after can be fired as a macro task then scheduling within a micro task should be fine at all times, right? |
This seems sketchy: // Ignoring 'effect' from the dependency array
// eslint-disable-next-line react-hooks/exhaustive-deps I think a more consistent (with other custom hooks) approach would be to remove the |
I went for the current approach so useEffect(effect, deps);
useDeferredEffect(effect, deps); |
@Andarist I think it would be difficult to have a 'one size fits all' option given that the solution would likely be dependent on the specific timing needs of the effect. That said, let's talk about event listeners. Let's say you want to start listening to events on the Microtask (
|
I agree. However - it seems that if we take those "race conditions" out of the picture then we are mostly OK with the timing of the
This is right and it's unfortunate for this use case :/ What about smth like this: // that lengthy list of arguments could potentially be refactored somehow
export default function useGlobalEventListener(
targetOrGetter
type,
listener,
options,
deps,
) {
useEffect(
function setup() {
const target = typeof targetOrGetter === 'function' ? targetOrGetter() : targetOrGetter
const currentEvent = window.event
// just a small memory leak prevention
const currentEventTimer = setTimeout(() => (currentEvent = null), 0)
const unbind = bind(target, {
type,
listener: function(...args) {
if (currentEvent && currentEvent === window.event) {
return
}
return listener.apply(this, args)
},
options
})
return function cleanup() {
clearTimeout(currentEventTimer)
unbind()
};
},
// Ignoring 'effect' from the dependency array
// eslint-disable-next-line react-hooks/exhaustive-deps
deps,
);
} |
Using Another way that could work for this particular use case is to utilize the event path + event phases.
React adds it's event listeners to the If you wanted to add an event listener to the An approach that would work would be to:
|
I know I could spin on this more, but that probably isn't super helpful to others. I think the big callout from this issue is that effects need to be designed with the understanding that they might be run synchronously, and they need to account for that! I'm not sure if there is a one size fits all solution (unless we had something like |
What we've done in Material-UI is keep attaching the event listener as normal but also attaching one for the capture phase that activates the handler. That way you make sure that the event listener only listens to new events e.g. https://github.com/mui-org/material-ui/blob/5687758c6bcaadc946d389e03df5fc9e9b482368/packages/mui-lab/src/internal/pickers/PickersPopper.tsx#L70-L89 That solution seemed more predictable as opposed to relying on the event loop. I guess you could even make it so that you only attach the actual listener during the capture phase instead of during React's effect phase. We've only tested this for handlers in the bubble phase. I don't think it'll work for handlers that need the capture phase. |
That is true - but there are no signs of it being removed any time soon. In fact, React itself is using this to determine a priority of an update. The workaround with an additional capture listener is also very creative 👍 |
useEffect(effect, deps);
useDeferredEffect(effect, deps); Right. I assumed that was the intent, but effect dependencies are easy to mess up. That's why we have a lint rule to enforce proper usage for built-in effects. (In the future we may also provide tools to automatically manage effect dependencies during compilation– for built-in effects). Those are the reasons why I would recommend third party hooks don't work with deps directly, and instead instruct people to use a built-in hook like |
Hi legends,
My mental model of
useEffect
is that schedules an asynchronous task to be executed after renderingThis view seems to be shared amongst a number of popular writings on the topic:
I have recently discovered two use cases where effects created with
useEffect
are synchronousstate
inside of a non-react event listenerstate
inside of aref
callbackHere is the stack during a flushed effect from the first example:
Having effects sometimes be synchronous is usually fine for most effects. However, there can be cases where having an effect running synchronously can be problematic. This recently caused a bug for a project I work on.
In an effect schedule with
useEffect
, we were adding a'click'
event listener to thewindow
in the ↑ bubble phase after somestate
changed. Because the effect was flushed synchronously, the new bubble phase event listener on thewindow
picked up the original'click'
event (which got picked up by react early on the event path)Tested
react
+react-dom
versions:16.14.0
17.0.2
I tried wrapping the
setState
in aunstable_batchedUpdates
but that did not impact the flushing of the effect.React 18
version:
18.0.0-alpha-f2c381131-20211004
First example: "setting react
state
inside of a non-react event listener"React 18 example
→ Different behaviour depending on
createRoot
+render
If using
ReactDOM.createRoot
then effects are flushed synchronously (the same as React 17)Interestingly, if you use
ReactDOM.render
with React 18, then effects are not flushed synchronously (different behaviour to React 17)Second example: "setting react
state
inside of aref
callback"React 18 example
createRoot
+render
Where to from here?
useEffect
sometimes synchronous? What should the behaviour be in React 18?useEffect
can something be synchronous; there are probably good reasons for them to be; and for most effects it should be fine. However, I do think it would be worth calling out that they can sometimes be synchronous as it can cause timing bugs. Perhaps a note in the docs would be sufficientThanks @aprilxyc and @Andarist for helping me troubleshoot this one!
The text was updated successfully, but these errors were encountered: