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

useRef: Warn about reading or writing mutable values during render #18545

Merged
merged 2 commits into from
Oct 19, 2020

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Apr 8, 2020

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 useTransition to no longer use a ref hook, but instead manage its own (stable) hook state.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 8, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 8, 2020

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:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Apr 8, 2020

Details of bundled changes.

Comparing: 51a3aa6...551985a

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% -0.0% 872.44 KB 872.46 KB 199.73 KB 199.7 KB NODE_DEV
ReactDOMForked-prod.js 🔺+0.1% 🔺+0.1% 383.49 KB 383.8 KB 70.76 KB 70.81 KB FB_WWW_PROD
react-dom-server.node.development.js 0.0% 0.0% 137.06 KB 137.06 KB 36.37 KB 36.37 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 117.72 KB 117.74 KB 37.94 KB 37.94 KB NODE_PROD
ReactDOMForked-profiling.js +0.1% +0.1% 398.04 KB 398.37 KB 73.27 KB 73.33 KB FB_WWW_PROFILING
react-dom-server.browser.development.js 0.0% 0.0% 143.14 KB 143.14 KB 36.57 KB 36.57 KB UMD_DEV
react-dom-server.node.production.min.js 0.0% 0.0% 20.2 KB 20.2 KB 7.58 KB 7.58 KB NODE_PROD
react-dom-test-utils.production.min.js 0.0% 0.0% 13.7 KB 13.7 KB 5.31 KB 5.32 KB UMD_PROD
ReactDOMTesting-dev.js 0.0% -0.0% 940.5 KB 940.52 KB 210.73 KB 210.71 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 66.27 KB 66.27 KB 18.83 KB 18.83 KB NODE_DEV
ReactDOMTesting-prod.js 0.0% -0.0% 384.57 KB 384.64 KB 72.36 KB 72.35 KB FB_WWW_PROD
react-dom-test-utils.production.min.js 0.0% 0.0% 13.68 KB 13.68 KB 5.26 KB 5.26 KB NODE_PROD
ReactTestUtils-dev.js +0.1% +0.1% 60.84 KB 60.91 KB 16.76 KB 16.78 KB FB_WWW_DEV
react-dom.development.js 0.0% -0.0% 916.83 KB 916.85 KB 202.27 KB 202.24 KB UMD_DEV
react-dom.production.min.js 0.0% -0.0% 117.62 KB 117.64 KB 38.64 KB 38.64 KB UMD_PROD
react-dom.profiling.min.js 0.0% -0.0% 121.52 KB 121.54 KB 39.87 KB 39.85 KB UMD_PROFILING
ReactDOMForked-dev.js +0.3% +0.3% 988.98 KB 991.71 KB 218.9 KB 219.53 KB FB_WWW_DEV
react-dom.profiling.min.js 0.0% 0.0% 121.8 KB 121.82 KB 39.12 KB 39.13 KB NODE_PROFILING
react-dom-server.browser.production.min.js 0.0% 0.0% 19.88 KB 19.88 KB 7.46 KB 7.46 KB UMD_PROD
ReactDOM-dev.js +0.3% +0.3% 977.29 KB 980.02 KB 217.31 KB 217.94 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 381.06 KB 381.36 KB 70.24 KB 70.29 KB FB_WWW_PROD
react-dom-server.browser.development.js 0.0% 0.0% 135.79 KB 135.79 KB 36.12 KB 36.12 KB NODE_DEV
ReactDOM-profiling.js +0.1% +0.1% 394.07 KB 394.37 KB 72.61 KB 72.67 KB FB_WWW_PROFILING
react-dom-server.browser.production.min.js 0.0% 0.0% 19.78 KB 19.78 KB 7.42 KB 7.42 KB NODE_PROD
ReactDOMServer-dev.js +0.1% +0.1% 145.44 KB 145.51 KB 37.28 KB 37.3 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% -0.0% 47.3 KB 47.3 KB 11.04 KB 11.04 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% 0.0% 71.46 KB 71.46 KB 19.34 KB 19.34 KB UMD_DEV

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-prod.js 🔺+0.1% 🔺+0.1% 240.1 KB 240.41 KB 42.44 KB 42.49 KB FB_WWW_PROD
react-art.development.js 0.0% -0.0% 664.4 KB 664.42 KB 141.61 KB 141.58 KB UMD_DEV
react-art.production.min.js 0.0% -0.0% 108.49 KB 108.51 KB 33.71 KB 33.7 KB UMD_PROD
react-art.development.js 0.0% -0.0% 566.82 KB 566.84 KB 123.81 KB 123.78 KB NODE_DEV
react-art.production.min.js 0.0% -0.0% 73.43 KB 73.45 KB 22.82 KB 22.82 KB NODE_PROD
ReactART-dev.js +0.4% +0.5% 628.77 KB 631.5 KB 133.55 KB 134.18 KB FB_WWW_DEV

ReactDOM: size: 0.0%, gzip: 0.0%

Size changes (stable)

Generated by 🚫 dangerJS against 551985a

@sizebot
Copy link

sizebot commented Apr 8, 2020

Details of bundled changes.

Comparing: 51a3aa6...551985a

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% -0.0% 907.99 KB 908.01 KB 206.28 KB 206.25 KB NODE_DEV
ReactDOMForked-prod.js 🔺+0.1% 🔺+0.1% 372.17 KB 372.48 KB 68.91 KB 68.96 KB FB_WWW_PROD
react-dom-server.node.development.js 0.0% 0.0% 138.57 KB 138.57 KB 36.58 KB 36.58 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 122.26 KB 122.28 KB 39.29 KB 39.3 KB NODE_PROD
ReactDOMForked-profiling.js +0.1% +0.1% 386.67 KB 386.99 KB 71.46 KB 71.5 KB FB_WWW_PROFILING
react-dom-server.browser.development.js 0.0% 0.0% 144.73 KB 144.73 KB 36.77 KB 36.77 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 13.71 KB 13.71 KB 5.32 KB 5.32 KB UMD_PROD
ReactDOMTesting-dev.js 0.0% -0.0% 912.16 KB 912.18 KB 205.21 KB 205.18 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 66.28 KB 66.28 KB 18.84 KB 18.84 KB NODE_DEV
ReactDOMTesting-prod.js 0.0% -0.0% 371.5 KB 371.58 KB 70.16 KB 70.15 KB FB_WWW_PROD
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 5.52 KB 5.52 KB 1.84 KB 1.84 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 13.7 KB 13.7 KB 5.26 KB 5.27 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 5.25 KB 5.25 KB 1.78 KB 1.78 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.3% 1.22 KB 1.22 KB 711 B 713 B UMD_PROD
ReactTestUtils-dev.js +0.1% +0.1% 60.84 KB 60.91 KB 16.77 KB 16.79 KB FB_WWW_DEV
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 4.78 KB 4.78 KB 1.68 KB 1.68 KB NODE_DEV
react-dom.development.js 0.0% -0.0% 954.14 KB 954.16 KB 208.84 KB 208.82 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.3% 1.01 KB 1.01 KB 615 B 617 B NODE_PROD
react-dom.production.min.js 0.0% 0.0% 122.09 KB 122.11 KB 40.01 KB 40.03 KB UMD_PROD
react-dom.profiling.min.js 0.0% -0.1% 127.36 KB 127.38 KB 41.71 KB 41.67 KB UMD_PROFILING
ReactDOMForked-dev.js +0.3% +0.3% 963.39 KB 966.13 KB 214.14 KB 214.77 KB FB_WWW_DEV
react-dom.profiling.min.js 0.0% -0.0% 127.71 KB 127.73 KB 40.96 KB 40.96 KB NODE_PROFILING
react-dom-server.browser.production.min.js 0.0% 0.0% 20.34 KB 20.34 KB 7.55 KB 7.55 KB UMD_PROD
ReactDOM-dev.js +0.3% +0.3% 951.7 KB 954.44 KB 212.61 KB 213.25 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 369.8 KB 370.11 KB 68.47 KB 68.51 KB FB_WWW_PROD
react-dom-server.browser.development.js 0.0% 0.0% 137.3 KB 137.3 KB 36.33 KB 36.33 KB NODE_DEV
ReactDOM-profiling.js +0.1% +0.1% 382.76 KB 383.07 KB 70.87 KB 70.92 KB FB_WWW_PROFILING
react-dom-server.browser.production.min.js 0.0% 0.0% 20.24 KB 20.24 KB 7.5 KB 7.5 KB NODE_PROD
ReactDOMServer-dev.js +0.1% +0.1% 141.41 KB 141.48 KB 36.26 KB 36.29 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% 0.0% 46.44 KB 46.44 KB 10.83 KB 10.83 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% 0.0% 71.47 KB 71.47 KB 19.35 KB 19.35 KB UMD_DEV

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-prod.js 🔺+0.1% 🔺+0.1% 232.96 KB 233.27 KB 41.18 KB 41.23 KB FB_WWW_PROD
react-art.development.js 0.0% -0.0% 691.3 KB 691.32 KB 146.62 KB 146.58 KB UMD_DEV
react-art.production.min.js 0.0% -0.0% 111.38 KB 111.4 KB 34.58 KB 34.58 KB UMD_PROD
react-art.development.js 0.0% -0.0% 592.46 KB 592.48 KB 128.74 KB 128.7 KB NODE_DEV
react-art.production.min.js 0.0% 0.0% 76.26 KB 76.29 KB 23.67 KB 23.67 KB NODE_PROD
ReactART-dev.js +0.4% +0.5% 618.76 KB 621.49 KB 131.52 KB 132.14 KB FB_WWW_DEV

ReactDOM: size: 0.0%, gzip: 0.0%

Size changes (experimental)

Generated by 🚫 dangerJS against 551985a

@markerikson
Copy link
Contributor

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)?

@gaearon
Copy link
Collaborator

gaearon commented Apr 9, 2020

Just to be clear, this only kicks if you both write and read while rendering? Just reading is safe?

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.

@gaearon
Copy link
Collaborator

gaearon commented Apr 9, 2020

Does this warning also only kick in if they both happen within the same function component execution (ie, resets after rendering is done)?

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.

@billyjanitsch
Copy link
Contributor

Am I right to think that this warning would be triggered by the recommended implementation of usePrevious?

@gaearon
Copy link
Collaborator

gaearon commented Apr 9, 2020

Looks like it.

Can you show some code snippets of how you use it?

@bvaughn bvaughn force-pushed the read-ref-during-render-warning branch 2 times, most recently from a1edb82 to b3dc63b Compare April 9, 2020 18:31
@billyjanitsch
Copy link
Contributor

@gaearon Sorry, just now finding time to write this up.

Can you show some code snippets of how you use it?

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. usePrevious is documented and has accordingly found its way into various popular hook technical writing and hook libraries, so I've found that, in practice, more developers are aware of it than of setting state during render and it often gets used for the same purpose. I know you'd want to steer people away from this but consider that the warning in this PR is not actionable for people using this for derived state who don't know that state can be set in render.

(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 usePrevious in product code. Maybe I haven't sufficiently considered alternatives. The general case is wanting to perform some side effect in response to a changed value where the effect also depends on the previous value.

Logging

Sometimes 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 usePrevious is doing here?

Warning

A 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 defaultValue for input) changes to indicate that the change will be ignored. The warning should include the old and new value.

#18547 is a nice change that lets us do it in render but only if we're using the native console.

Debugging

While 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. usePrevious serves all of these cases nicely without much code and I haven't been able to imagine a similarly simple alternative.

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 18, 2020

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

This could be rewritten to manually read/write to the ref in the effect, but how would you write something reusable that encapsulates what usePrevious is doing here?

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

A 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 defaultValue for input) changes to indicate that the change will be ignored.

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.

Debugging

I don't have a solution for this use case that wouldn't violate the warning this PR explores.

@TrySound
Copy link
Contributor

We actively use constant hook to get the first rendered value. It's also in docs
https://reactjs.org/docs/hooks-faq.html#how-to-create-expensive-objects-lazily

export const useConstant = <T>(fn: () => T): T => {
  const ref = React.useRef<null | {| value: T |}>(null);
  if (ref.current == null) {
    // we instantiate { value } to not conflict with returned null
    ref.current = { value: fn() };
  }
  return ref.current.value;
};

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 19, 2020

@TrySound This PR accounts for that pattern too. (The first time a value is set from null to something else doesn't count against the "unsafe to read later" rule.)

@dai-shi
Copy link
Contributor

dai-shi commented Apr 24, 2020

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.

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>

khmm12 added a commit to khmm12/react-stripe-js that referenced this pull request May 14, 2020
Refs cannot be mutated and used to update state in the same time in rendering phase. As this is side-effect, it can produce various bugs in concurrent mode.

In StrictMode Elements doesn't do transition from null to valid stripe instance. As in StrictMode React renders twice, `final` ref becomes `true`, but `ctx` state isn't changed.

References:
https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects
facebook/react#18003
facebook/react#18545
khmm12 added a commit to khmm12/react-stripe-js that referenced this pull request Jun 8, 2020
Refs cannot be mutated and used to update state in the same time in rendering phase. As this is side-effect, it can produce various bugs in concurrent mode.

In StrictMode Elements doesn't do transition from null to valid stripe instance. As in StrictMode React renders twice, `final` ref becomes `true`, but `ctx` state isn't changed.

References:
https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects
facebook/react#18003
facebook/react#18545
khmm12 added a commit to khmm12/react-stripe-js that referenced this pull request Jun 8, 2020
Refs cannot be mutated and used to update state in the same time in rendering phase. As this is side-effect, it can produce various bugs in concurrent mode.

In StrictMode Elements doesn't do transition from null to valid stripe instance. As in StrictMode React renders twice, `final` ref becomes `true`, but `ctx` state isn't changed.

References:
https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects
facebook/react#18003
facebook/react#18545
khmm12 added a commit to khmm12/react-stripe-js that referenced this pull request Jun 8, 2020
Refs cannot be mutated and used to update state in the same time in rendering phase. As this is side-effect, it can produce various bugs in concurrent mode.

In StrictMode Elements doesn't do transition from null to valid stripe instance. As in StrictMode React renders twice, `final` ref becomes `true`, but `ctx` state isn't changed.

References:
https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects
facebook/react#18003
facebook/react#18545
if (!hasBeenInitialized) {
didCheckForLazyInit = true;
lazyInitGetterStack = getCallerStackFrame();
} else if (currentlyRenderingFiber !== null && !didWarnAboutRead) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it's read inside a class component? Or written to. Should it warn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undefined I guess. I could expand the warning to include class components as well, if you think that's worth doing.

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.
@bvaughn bvaughn force-pushed the read-ref-during-render-warning branch from e4ba7cf to 552900b Compare October 19, 2020 19:20
@bvaughn
Copy link
Contributor Author

bvaughn commented Oct 19, 2020

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 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.

• Changed tests to use pragma
• Renamed feature flag
@bvaughn bvaughn force-pushed the read-ref-during-render-warning branch from 552900b to 551985a Compare October 19, 2020 19:34
@bvaughn bvaughn merged commit c59c3df into facebook:master Oct 19, 2020
@bvaughn bvaughn deleted the read-ref-during-render-warning branch October 19, 2020 20:05
@Andarist
Copy link
Contributor

Andarist commented Oct 20, 2020

would this warn for this type of situation? https://codesandbox.io/s/agitated-rubin-x2c1l?file=/src/index.js:380-595

Yes, in its current form.

Isn't this a classic lazy init pattern that is being tested here? 🤔

You could also use memo, like so:

Isn't this unsafe for this use case as useMemo doesn't have semantic guarantees and can be just freely reinitialized when React decides to do so (which includes recomputation during Fast Refresh)?

@bvaughn
Copy link
Contributor Author

bvaughn commented Oct 20, 2020

Isn't this a classic lazy init pattern that is being tested here? 🤔

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 ref.current to createPortal).

Isn't this unsafe for this use case as useMemo doesn't have semantic guarantees and can be just freely reinitialized when React decides to do so (which includes recomputation during Fast Refresh)?

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.

@josepot
Copy link

josepot commented Oct 20, 2020

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 ref.current to createPortal).

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?

@bvaughn
Copy link
Contributor Author

bvaughn commented Oct 20, 2020

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.

@josepot
Copy link

josepot commented Oct 20, 2020

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.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Oct 27, 2020
Summary:
This sync includes the following changes:
- **[eaaf4cbce](facebook/react@eaaf4cbce )**: 17.0.1 //<Dan Abramov>//
- **[6f62abb58](facebook/react@6f62abb58 )**: Remove usage of Array#fill ([#20071](facebook/react#20071)) //<Dan Abramov>//
- **[40cfe1f48](facebook/react@40cfe1f48 )**: Update CHANGELOG.md //<Dan Abramov>//
- **[f021a983a](facebook/react@f021a983a )**: Bump versions for 17 ([#20062](facebook/react#20062)) //<Dan Abramov>//
- **[d1bb4d851](facebook/react@d1bb4d851 )**: Profiler: Include ref callbacks in onCommit duration ([#20060](facebook/react#20060)) //<Brian Vaughn>//
- **[c59c3dfe5](facebook/react@c59c3dfe5 )**: useRef: Warn about reading or writing mutable values during render ([#18545](facebook/react#18545)) //<Brian Vaughn>//
- **[7b6cac952](facebook/react@7b6cac952 )**: Improved Profiler commit hooks test ([#20053](facebook/react#20053)) //<Brian Vaughn>//
- **[dfb6a4033](facebook/react@dfb6a4033 )**: [Fast Refresh] Fix crashes caused by rogue Proxies ([#20030](facebook/react#20030)) ([#20039](facebook/react#20039)) //<Kai Riemann>//
- **[37cb732c5](facebook/react@37cb732c5 )**: Use bitwise OR to define flag masks ([#20044](facebook/react#20044)) //<Andrew Clark>//
- **[eb3181e77](facebook/react@eb3181e77 )**: Add Visibility flag for hiding/unhiding trees ([#20043](facebook/react#20043)) //<Andrew Clark>//
- **[0dd809bdf](facebook/react@0dd809bdf )**: Remove last schedulePassiveEffectCallback call ([#20042](facebook/react#20042)) //<Andrew Clark>//
- **[8df7b7911](facebook/react@8df7b7911 )**: Remove Passive flag from "before mutation" phase ([#20038](facebook/react#20038)) //<Andrew Clark>//
- **[c57fe4a2c](facebook/react@c57fe4a2c )**: ReactIs.isValidElementType Unit Test extended with PureComponent case ([#20033](facebook/react#20033)) //<adasq>//
- **[02da938fd](facebook/react@02da938fd )**: Don't double-invoke effects in legacy roots ([#20028](facebook/react#20028)) //<Brian Vaughn>//
- **[d95c4938d](facebook/react@d95c4938d )**: [EventSystem] Revise onBeforeBlur propagation mechanics ([#20020](facebook/react#20020)) //<Dominic Gannaway>//
- **[f46a80ae1](facebook/react@f46a80ae1 )**: Update outdated links and fix two broken links  ([#19985](facebook/react#19985)) //<Saikat Guha>//
- **[0a4c7c565](facebook/react@0a4c7c565 )**: [Flight] Don't warn for key, but error for ref ([#19986](facebook/react#19986)) //<Sebastian Markbåge>//
- **[993ca533b](facebook/react@993ca533b )**: Enable eager listeners statically ([#19983](facebook/react#19983)) //<Dan Abramov>//
- **[40c52de96](facebook/react@40c52de96 )**: [Flight] Add Runtime Errors for Non-serializable Values ([#19980](facebook/react#19980)) //<Sebastian Markbåge>//
- **[1992d9730](facebook/react@1992d9730 )**: Revert "Temporarily disable Profiler commit hooks flag ([#19900](facebook/react#19900))" ([#19960](facebook/react#19960)) //<Brian Vaughn>//
- **[44d39c4d7](facebook/react@44d39c4d7 )**: Removed skip-error-boundaries modifications from old fork ([#19961](facebook/react#19961)) //<Brian Vaughn>//
- **[cc77be957](facebook/react@cc77be957 )**: Remove unnecessary error overriding in ([#19949](facebook/react#19949)) //<Paul Doyle>//
- **[97625272a](facebook/react@97625272a )**: Debug tracing tests for CPU bound suspense ([#19943](facebook/react#19943)) //<Brian Vaughn>//
- **[43363e279](facebook/react@43363e279 )**: Fix codestyle for typeof comparison ([#19928](facebook/react#19928)) //<Eugene Maslovich>//
- **[5427b4657](facebook/react@5427b4657 )**: Temporarily disable Profiler commit hooks flag ([#19900](facebook/react#19900)) //<Brian Vaughn>//
- **[1faf9e3dd](facebook/react@1faf9e3dd )**: Suspense for CPU-bound trees ([#19936](facebook/react#19936)) //<Andrew Clark>//
- **[7f08e908b](facebook/react@7f08e908b )**: Fix missing context to componentDidMount() when double-invoking lifecycles ([#19935](facebook/react#19935)) //<Brian Vaughn>//
- **[9198a5cec](facebook/react@9198a5cec )**: Refactor layout effect methods ([#19895](facebook/react#19895)) //<Brian Vaughn>//
- **[ba82eea38](facebook/react@ba82eea38 )**: Remove disableSchedulerTimeoutInWorkLoop flag ([#19902](facebook/react#19902)) //<Andrew Clark>//
- **[c63741fb3](facebook/react@c63741fb3 )**: offscreen double invoke effects ([#19523](facebook/react#19523)) //<Luna Ruan>//
- **[c6917346f](facebook/react@c6917346f )**: Fixed broken Profiler test ([#19894](facebook/react#19894)) //<Brian Vaughn>//
- **[87c023b1c](facebook/react@87c023b1c )**: Profiler onRender only called when we do work ([#19885](facebook/react#19885)) //<Brian Vaughn>//
- **[81aaee56a](facebook/react@81aaee56a )**: Don't call onCommit et al if there are no effects ([#19863](facebook/react#19863)) //<Andrew Clark>//
- **[7355bf575](facebook/react@7355bf575 )**: Consolidate commit phase hook functions ([#19864](facebook/react#19864)) //<Andrew Clark>//
- **[bc6b7b6b1](facebook/react@bc6b7b6b1 )**: Don't trigger lazy in DEV during element creation ([#19871](facebook/react#19871)) //<Dan Abramov>//
- **[a774502e0](facebook/react@a774502e0 )**: Use single quotes in getComponentName return ([#19873](facebook/react#19873)) //<Gustavo Saiani>//
- **[8b2d3783e](facebook/react@8b2d3783e )**: Use Passive flag to schedule onPostCommit ([#19862](facebook/react#19862)) //<Andrew Clark>//
- **[50d9451f3](facebook/react@50d9451f3 )**: Improve DevTools editing interface ([#19774](facebook/react#19774)) //<Brian Vaughn>//
- **[6fddca27e](facebook/react@6fddca27e )**: Remove passive intervention flag ([#19849](facebook/react#19849)) //<Dan Abramov>//
- **[36df9185c](facebook/react@36df9185c )**: chore(docs): Removed outdated comment about fb.me link  ([#19830](facebook/react#19830)) //<Adnaan Bheda>//
- **[16fb2b6f9](facebook/react@16fb2b6f9 )**: Moved resetChildLanes into complete work ([#19836](facebook/react#19836)) //<Brian Vaughn>//
- **[cc581065d](facebook/react@cc581065d )**: eslint-plugin-react-hooks@4.1.2 //<Dan Abramov>//
- **[0044805c8](facebook/react@0044805c8 )**: Update CHANGELOG.md //<Dan Abramov>//
- **[0f70d4dd6](facebook/react@0f70d4dd6 )**: Consider components in jsx as missing dependencies in typescript-eslint/parser@4.x ([#19815](facebook/react#19815)) //<Sebastian Silbermann>//
- **[84558c61b](facebook/react@84558c61b )**: Don't visit passive effects during layout phase ([#19809](facebook/react#19809)) //<Andrew Clark>//
- **[ad8a0a8cd](facebook/react@ad8a0a8cd )**: eslint-plugin-react-hooks@4.1.1 //<Dan Abramov>//
- **[77544a0d6](facebook/react@77544a0d6 )**: Update CHANGELOG.md //<Dan Abramov>//
- **[ed4fdfc73](facebook/react@ed4fdfc73 )**: test(eslint-plugin-react-hooks): Run with TS parsers >= 2.x ([#19792](facebook/react#19792)) //<Sebastian Silbermann>//
- **[cd75f93c0](facebook/react@cd75f93c0 )**: eslint-plugin-react-hooks: fix compatibility with typescript-eslint/parser@4.0.0+ ([#19751](facebook/react#19751)) //<Matthias Schiffer>//
- **[781212aab](facebook/react@781212aab )**: Remove double space in test name ([#19762](facebook/react#19762)) //<Gustavo Saiani>//
- **[e7b255341](facebook/react@e7b255341 )**: Internal `act`: Flush timers at end of scope ([#19788](facebook/react#19788)) //<Andrew Clark>//
- **[d17086c7c](facebook/react@d17086c7c )**: Decouple public, internal act implementation ([#19745](facebook/react#19745)) //<Andrew Clark>//
- **[d38ec17b1](facebook/react@d38ec17b1 )**: [Flight] Set dispatcher for duration of performWork() ([#19776](facebook/react#19776)) //<Joseph Savona>//
- **[4f3f7eeb7](facebook/react@4f3f7eeb7 )**: Bugfix: Effect clean up when deleting suspended tree ([#19752](facebook/react#19752)) //<Andrew Clark>//
- **[7baf9d412](facebook/react@7baf9d412 )**: Combine Flags and SubtreeFlags types ([#19775](facebook/react#19775)) //<Andrew Clark>//
- **[166544360](facebook/react@166544360 )**: Rename effect fields ([#19755](facebook/react#19755)) //<Andrew Clark>//
- **[708fa77a7](facebook/react@708fa77a7 )**: Decrease expiration time of input updates ([#19772](facebook/react#19772)) //<Andrew Clark>//
- **[36df483af](facebook/react@36df483af )**: Add feature flag to disable scheduler timeout in work loop ([#19771](facebook/react#19771)) //<Ricky>//
- **[bcc0aa463](facebook/react@bcc0aa463 )**: Revert "Revert "Remove onScroll bubbling flag ([#19535](facebook/react#19535))" ([#19655](facebook/react#19655))" ([#19761](facebook/react#19761)) //<Dan Abramov>//
- **[99cae887f](facebook/react@99cae887f )**: Add failing test for passive effect cleanup functions and memoized components ([#19750](facebook/react#19750)) //<Brian Vaughn>//
- **[2cfd73c4d](facebook/react@2cfd73c4d )**: Fix typo in comment (Noticable→Noticeable) ([#19737](facebook/react#19737)) //<Ikko Ashimine>//
- **[53e622ca7](facebook/react@53e622ca7 )**: Fix instances of function declaration after return ([#19733](facebook/react#19733)) //<Bhumij Gupta>//
- **[b7d18c4da](facebook/react@b7d18c4da )**: Support Babel's envName option in React Refresh plugin ([#19009](facebook/react#19009)) //<Kevin Weber>//
- **[1f38dcff6](facebook/react@1f38dcff6 )**: Remove withSuspenseConfig ([#19724](facebook/react#19724)) //<Andrew Clark>//
- **[1396e4a8f](facebook/react@1396e4a8f )**: Fixes eslint warning when node type is ChainExpression ([#19680](facebook/react#19680)) //<Pascal Fong Kye>//
- **[a8500be89](facebook/react@a8500be89 )**: Add `startTransition` as a known stable method ([#19720](facebook/react#19720)) //<Andrew Clark>//
- **[380dc95de](facebook/react@380dc95de )**: Revert "Append text string to <Text> error message ([#19581](facebook/react#19581))" ([#19723](facebook/react#19723)) //<Timothy Yung>//
- **[ddd1faa19](facebook/react@ddd1faa19 )**: Remove config argument from useTransition ([#19719](facebook/react#19719)) //<Andrew Clark>//
- **[92fcd46cc](facebook/react@92fcd46cc )**: Replace SuspenseConfig object with an integer ([#19706](facebook/react#19706)) //<Andrew Clark>//
- **[b754caaaf](facebook/react@b754caaaf )**: Enable eager listeners in open source ([#19716](facebook/react#19716)) //<Dan Abramov>//
- **[c1ac05215](facebook/react@c1ac05215 )**: [Flight] Support more element types and Hooks for Server and Hybrid Components ([#19711](facebook/react#19711)) //<Dan Abramov>//
- **[1eaafc9ad](facebook/react@1eaafc9ad )**: Clean up timeoutMs-related implementation details ([#19704](facebook/react#19704)) //<Andrew Clark>//
- **[8da0da093](facebook/react@8da0da093 )**: Disable timeoutMs argument ([#19703](facebook/react#19703)) //<Andrew Clark>//
- **[60ba723bf](facebook/react@60ba723bf )**: Add SuspenseList to devTools ([#19684](facebook/react#19684)) //<Ben Pernick>//
- **[5564f2c95](facebook/react@5564f2c95 )**: Add React.startTransition ([#19696](facebook/react#19696)) //<Ricky>//
- **[c4e0768d7](facebook/react@c4e0768d7 )**: Remove unused argument from `finishConcurrentRender` ([#19689](facebook/react#19689)) //<inottn>//
- **[848bb2426](facebook/react@848bb2426 )**: Attach Listeners Eagerly to Roots and Portal Containers ([#19659](facebook/react#19659)) //<Dan Abramov>//
- **[d2e914ab4](facebook/react@d2e914ab4 )**: Remove remaining references to effect list ([#19673](facebook/react#19673)) //<Andrew Clark>//
- **[d6e433899](facebook/react@d6e433899 )**: Use Global Render Timeout for CPU Suspense ([#19643](facebook/react#19643)) //<Sebastian Markbåge>//
- **[64ddef44c](facebook/react@64ddef44c )**: Revert "Remove onScroll bubbling flag ([#19535](facebook/react#19535))" ([#19655](facebook/react#19655)) //<Dan Abramov>//
- **[dd651df05](facebook/react@dd651df05 )**: Keep onTouchStart, onTouchMove, and onWheel passive ([#19654](facebook/react#19654)) //<Dan Abramov>//
- **[b8fa09e9e](facebook/react@b8fa09e9e )**: provide profiling bundle for react-reconciler ([#19559](facebook/react#19559)) //<Julien Gilli>//
- **[23595ff59](facebook/react@23595ff59 )**: Add missing param to safelyCallDestroy() ([#19638](facebook/react#19638)) //<Brian Vaughn>//
- **[ee409ea3b](facebook/react@ee409ea3b )**: change destroy to safelyCallDestroy ([#19605](facebook/react#19605)) //<Luna Ruan>//
- **[bcca5a6ca](facebook/react@bcca5a6ca )**: Always skip unmounted/unmounting error boundaries ([#19627](facebook/react#19627)) //<Brian Vaughn>//
- **[1a41a196b](facebook/react@1a41a196b )**: Append text string to <Text> error message ([#19581](facebook/react#19581)) //<Timothy Yung>//
- **[e4afb2fdd](facebook/react@e4afb2fdd )**: eslint-plugin-react-hooks@4.1.0 //<Dan Abramov>//
- **[ced05c46c](facebook/react@ced05c46c )**: Update CHANGELOG.md //<Dan Abramov>//
- **[702fad4b1](facebook/react@702fad4b1 )**: refactor fb.me redirect link to reactjs.org/link ([#19598](facebook/react#19598)) //<CY Lim>//
- **[49cd77d24](facebook/react@49cd77d24 )**: fix: leak strict mode with UMD builds ([#19614](facebook/react#19614)) //<Toru Kobayashi>//
- **[ffb749c95](facebook/react@ffb749c95 )**: Improve error boundary handling for unmounted subtrees ([#19542](facebook/react#19542)) //<Brian Vaughn>//
- **[9b35dd2fc](facebook/react@9b35dd2fc )**: Permanently removed component stacks from scheduling profiler data ([#19615](facebook/react#19615)) //<Brian Vaughn>//
- **[3f8115cdd](facebook/react@3f8115cdd )**: Remove `didTimeout` check from work loop //<Andrew Clark>//
- **[9abc2785c](facebook/react@9abc2785c )**: Remove wasteful checks from `shouldYield` //<Andrew Clark>//
- **[1d5e10f70](facebook/react@1d5e10f70 )**: [eslint-plugin-react-hooks] Report constant constructions ([#19590](facebook/react#19590)) //<Jordan Eldredge>//
- **[dab0854c5](facebook/react@dab0854c5 )**: Move commit passive unmount/mount to CommitWork ([#19599](facebook/react#19599)) //<Sebastian Markbåge>//
- **[ccb6c3945](facebook/react@ccb6c3945 )**: Remove unused argument ([#19600](facebook/react#19600)) //<inottn>//
- **[629125555](facebook/react@629125555 )**: [Scheduler] Re-throw unhandled errors ([#19595](facebook/react#19595)) //<Andrew Clark>//
- **[b8ed6a1aa](facebook/react@b8ed6a1aa )**: [Scheduler] Call postTask directly ([#19551](facebook/react#19551)) //<Andrew Clark>//
- **[ce37bfad5](facebook/react@ce37bfad5 )**: Remove resolutions from test renderer package.json ([#19577](facebook/react#19577)) //<Dan Abramov>//
- **[2704bb537](facebook/react@2704bb537 )**: Add ReactVersion to SchedulingProfiler render scheduled marks ([#19553](facebook/react#19553)) //<Kartik Choudhary>//
- **[0c52e24cb](facebook/react@0c52e24cb )**: Support inner component _debugOwner in memo ([#19556](facebook/react#19556)) //<Brian Vaughn>//
- **[0cd9a6de5](facebook/react@0cd9a6de5 )**: Parallelize Jest in CI ([#19552](facebook/react#19552)) //<Andrew Clark>//
- **[a63893ff3](facebook/react@a63893ff3 )**: Warn about undefined return value for memo and forwardRef ([#19550](facebook/react#19550)) //<Brian Vaughn>//
- **[32ff42868](facebook/react@32ff42868 )**: Add feature flag for setting update lane priority ([#19401](facebook/react#19401)) //<Ricky>//
- **[5bdd4c8c6](facebook/react@5bdd4c8c6 )**: Remove unused argument from call to jest method ([#19546](facebook/react#19546)) //<Gustavo Saiani>//
- **[a5fed98a9](facebook/react@a5fed98a9 )**: Register more node types that are used later as JSXIdentifiers ([#19514](facebook/react#19514)) //<Mateusz Burzyński>//
- **[f77c7b9d7](facebook/react@f77c7b9d7 )**: Re-add discrete flushing timeStamp heuristic (behind flag) ([#19540](facebook/react#19540)) //<Dominic Gannaway>//

Changelog: [general] [feature] Upgrade to React 17

Reviewed By: cpojer

Differential Revision: D24491201

fbshipit-source-id: c947da9dcccbd614e9dc58f3339b63e24829aca7
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
…acebook#18545)

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 useTransition to no longer use a ref hook, but instead manage its own (stable) hook state.
csm-thu added a commit to Cosmo-Tech/azure-sample-webapp that referenced this pull request Jan 5, 2022
Bug was probably caused by writing/reading during render, it seems to be fixed by using
a lazy-initialization pattern

c.f. facebook/react#18545
c.f. https://reactjs.org/docs/hooks-faq.html#how-to-create-expensive-objects-lazily
csm-thu added a commit to Cosmo-Tech/azure-sample-webapp that referenced this pull request Jan 6, 2022
Bug was probably caused by writing/reading during render, it seems to be fixed by using
a lazy-initialization pattern

c.f. facebook/react#18545
c.f. https://reactjs.org/docs/hooks-faq.html#how-to-create-expensive-objects-lazily
csm-thu added a commit to Cosmo-Tech/azure-sample-webapp that referenced this pull request Jan 6, 2022
Bug was probably caused by writing/reading during render, it seems to be fixed by using
a lazy-initialization pattern

c.f. facebook/react#18545
c.f. https://reactjs.org/docs/hooks-faq.html#how-to-create-expensive-objects-lazily
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.