-
Notifications
You must be signed in to change notification settings - Fork 220
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
[react-hooks] Fix mutating refs during render phase or asynchronously #1813
Conversation
8a2dfa0
to
1b7d9dd
Compare
79aa03f
to
1189913
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Could you please update the readme to include useIsomorphicLayoutEffect
.
typeof window.document.createElement !== 'undefined'; | ||
|
||
/** | ||
* A hook that resolves to useEffect on the server and useLayoutEffect on the client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be a server effect for the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you're referring to. Are you talking about react-effect
?
This change isn't related to running effects on the server. useEffect
does not run in a server-side environment. useLayoutEffect
throws a warning when used in a server-side environment, which is why we only invoke it when on the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, words. resolves to
misread that. 🙇
If the saved callback is updated in a normal useEffect instead of useLayoutEffect, the interval or timeout can be invoked with a stale callback function.
ee0ea0c
to
4197302
Compare
Description
Updating mutable references to callbacks
The
useEffect
hook runs asynchronously, after the rendering phase of a component is complete. When storing values in a mutable ref that should point to the latest argument passed to a component or hook, there can be a window of time between when the commit phase is kicked off and when the asyncuseEffect
function is called where it points to a stale callback.To avoid this, we need to use
useLayoutEffect
to make sure to synchronously update the ref to the latest value during the commit phase.useLayoutEffect
cannot be used in a server-side environment, so we need to also introduce theuseIsomorphicLayoutEffect
hook to safely be able to use this strategy in a server-side rendered application.We need to do this inside
useLayoutEffect
rather than directly in the body of the component for reasons described below.Mutating refs in the render phase
Refs should never be mutated while rendering. The
useLazyRef
component has been updated to a different strategy that does not involve mutating refs during the render phase of a component.This is currently more of a technicality, as with current versions of React it is technically safe to do this, but this will no longer be safe with some of the upcoming changes that are coming to React.
As a rule of thumb, React components should be pure. They should only calculate the output of the next render or invoke hooks. Updating a ref (as well as updating state) shouldn’t be performed inside the immediate scope of a component’s render cycle for that reason.
In the future, React could decide to pause rendering a component, and potentially abort completely and re-start from scratch. Other examples include the
useTransition
hook of the Suspense API and future work around built-in animations.Rendering should be treated as something that needs to be sandboxed and have no side-effects. You don't want the fact that React tried rendering something to leak out and have side-effects on mutable refs, for the same reason why you don't want to write to mutable state in a multithreaded environment. Exactly because it's hard to think about what can go wrong.
Next steps
We should create a linting rule against mutating refs in the body of functional components.
Type of change
@shopify/react-hooks
Patch: Bug (non-breaking change which fixes an issue)@shopify/react-hooks
Minor: New feature (non-breaking change which adds functionality)Checklist