useRef: Warn about reading or writing mutable values during render #18545
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 551985a:
|
Details of bundled changes.Comparing: 51a3aa6...551985a react-dom
react-art
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
Details of bundled changes.Comparing: 51a3aa6...551985a react-dom
react-art
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (experimental) |
ee03c74 to
b1d648c
Compare
|
Just to be clear, this only kicks if you both write and read while rendering? Just reading is safe? Does this warning also only kick in if they both happen within the same function component execution (ie, resets after rendering is done)? |
No, it’s reading itself that is a problem. Since it’s effectively the same as reading from a random global variable. It is non-deterministic because whatever you’re going to get depends on when render was called. If React calls render at a slightly different time you can have a different result. |
Can you explain this pattern? First, it’s hard to guarantee any resetting. You would have to try/finally your entire component. Second, if you only need a ref value temporarily during render, you could have used a regular variable. |
packages/react-reconciler/src/__tests__/useRef-test.internal.js
Outdated
Show resolved
Hide resolved
|
Am I right to think that this warning would be triggered by the recommended implementation of |
|
Looks like it. Can you show some code snippets of how you use it? |
a1edb82 to
b3dc63b
Compare
|
@gaearon Sorry, just now finding time to write this up.
Tbh, I most often see it used for derived state, which I know is better served by this pattern (if at all) since it avoids a multi-pass render. However, the ability to set state during render is only ever mentioned once in a "Hooks FAQ" answer and nowhere in the API reference. (Btw, I know that "React changes too often" chatter can be very frustrating given the team's focus on backwards compatibility, but I'm sure you can also understand that it could be discouraging to use a pattern sanctioned by the docs -- so much so that "it’s possible that in the future React will provide a usePrevious Hook out of the box since it’s a relatively common use case" -- only to get an error about it a few months later. I don't mean to be discouraging and I don't feel that way myself, but I hope this can give you a bit more empathy or patience towards folks who do.) That said, I think I've seen a few legit uses of LoggingSometimes we want to log when a value has changed, due to a user action or external event (we don't care by which mechanism). Often we want to log the old value as well as the new one, because we're interested in the transition. We generally only care to log the change if it actually got painted to the screen. For instance, imagine a UI where the width of a panel can be resized by the user. const {width} = props
const prevWidth = usePrevious(width)
useEffect(() => {
if (width !== prevWidth) {
log('resize', {width, prevWidth})
}
}, [width, prevWidth])This could be rewritten to manually read/write to the ref in the effect, but how would you write something reusable that encapsulates what WarningA similar example is adding a dev-only warning when a prop that should be static (e.g. the initialization value of an uncontrolled component, like #18547 is a nice change that lets us do it in render but only if we're using the native DebuggingWhile debugging, sometimes we care about a render in which some value has changed from the previous commit. For example, we want to pause execution if a value has changed so we can poke around the scope, or we just want to be informed if a value changed, because we didn't expect that. Maybe we even want to know if a value changed twice across two commits. |
|
Thanks for the detailed response, @billyjanitsch 🙇 As we develop concurrent mode APIs, and see them used more broadly at Facebook, we learn about patterns that can cause problems that we didn't know about initially. It's understandable that it can be frustrating when our guidance changes. For what it's worth, we do try to share our latest way of thinking about these APIs with the larger open source community as our understanding evolves. We'll also be working on a pretty substantial overhaul of the documentation over the next couple of months too, which will hopefully help. I should have taken the time to write a more detailed description in this PR. It was meant for discussion/consideration on the team more than anything. At this point, I don't think we'll be able to move forward with this PR. There are probably too many pre-existing cases that would cause it to fire. We may end up investigating a lint rule instead, since that could be configured/disabled. Logging
I think you could do something like that with a custom hook too, unless I'm misunderstanding. function useChangeCallback(value, onChange) {
const prevValueRef = useRef(value);
useEffect(() => {
const prevValue = prevValueRef.current;
if (prevValue !== value) {
prevValueRef.current = value;
onChange(value, prevValue);
}
});
}Warning
That's possible to do even with this warning. function Example({ propThatShouldNotChange }) {
const stableRef = useRef(propThatShouldNotChange);
if (stableRef.current !== propThatShouldNotChange) {
// Warn
}
}Because you never write to the ref (other than the initial value) reading is fine. DebuggingI don't have a solution for this use case that wouldn't violate the warning this PR explores. |
|
We actively use constant hook to get the first rendered value. It's also in docs |
|
@TrySound This PR accounts for that pattern too. (The first time a value is set from |
It would be nice for us to intentionally test our apps and libs. What about opting in with StrictMode? Is it feasible? Could it be easier (and more correct) than investigating a new eslint rule? <StrictMode warnReadingMutableRefValue> |
| if (!hasBeenInitialized) { | ||
| didCheckForLazyInit = true; | ||
| lazyInitGetterStack = getCallerStackFrame(); | ||
| } else if (currentlyRenderingFiber !== null && !didWarnAboutRead) { |
There was a problem hiding this comment.
What if it's read inside a class component? Or written to. Should it warn?
There was a problem hiding this comment.
Undefined I guess. I could expand the warning to include class components as well, if you think that's worth doing.
packages/react-reconciler/src/__tests__/useRef-test.internal.js
Outdated
Show resolved
Hide resolved
Reading or writing a ref value during render is only safe if you are implementing the lazy initialization pattern. Other types of reading are unsafe as the ref is a mutable source. Other types of writing are unsafe as they are effectively side effects.
e4ba7cf to
552900b
Compare
|
Hey everyone 👋 I am going to merge this PR and roll it out behind a feature flag within Facebook to assess its impact, e.g. how many warnings does it trigger, how many patterns (like I wouldn't recommend upgrading existing code until we have published recommended alternative patterns– but if it's easily done, avoid it in new code, you're aware of it now. |
• Changed tests to use pragma • Renamed feature flag
552900b to
551985a
Compare
Isn't this a classic lazy init pattern that is being tested here? 🤔
Isn't this unsafe for this use case as |
The reason that sandbox would warn with the semantics on this PR is not because of the lazy init pattern, but because of the read further down on line 60 (passing
My code example above had a silly mistake in the deps array. Should have been this: const container = useMemo(() => document.createElement("div"), []);
useLayoutEffect(() => {
modalRoot.append(container);
return () => container.remove();
}, [container]);If React did "forget" the memoized element between renders, the effect would clean up the old one and reparent the new one. |
But since that ref would always yield the same value, that would be a "false positive", correct? I mean, as long as the render function always behaves the same for the same props, state and context, then it's ok to read a value from a ref, right? |
|
Yes, it would be a false positive in this case. The danger with refs is that they can be passed around and mutated outside of React's awareness (unlike state/reducer). We could change the warning semantics to only warn on read if the ref was mutated after being initialized (not including lazy-init pattern). The danger there is that maybe there's a code path that triggers a second mutation that just doesn't run often. Maybe you'd rarely or never see it in dev. So you have a false sense of confidence that your component is safe, when really there's an unsafe read that might cause a bug in production if this infrequent code path gets triggered. |
Sorry, my intention was not to criticize the work done on this PR. I agree that it's better to err on the side of caution. I made that comment because I wasn't sure if you were proposing those alternatives because you deemed the original code to be unsafe or because you wanted to highlight alternative implementations that accomplished the same thing while avoiding warnings. Thanks a lot for clarifying that. |
This warning is being tested within Facebook behind a feature flag to assess its impact, (e.g. how many warnings does it trigger, how many patterns (like
usePrevious) does it flush out, do they all have safe alternate patterns, etc.)I wouldn't recommend upgrading existing code until we have published recommended alternative patterns– but if it's easily done, avoid it in new code, you're aware of it now.
Reading or writing a ref value during render is only safe if you are implementing the lazy initialization pattern.
Other types of reading are unsafe as the ref is a mutable source.
Other types of writing are unsafe as they are effectively side effects.
This change also refactors
useTransitionto no longer use a ref hook, but instead manage its own (stable) hook state.