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

Bug: StrictMode is not preventing side effects #28898

Open
theKashey opened this issue Apr 24, 2024 · 5 comments
Open

Bug: StrictMode is not preventing side effects #28898

theKashey opened this issue Apr 24, 2024 · 5 comments

Comments

@theKashey
Copy link
Contributor

theKashey commented Apr 24, 2024

React version: any with StrictMode

Steps To Reproduce

  1. Create a few components with improper or absent memoization
  2. Create useEffect with unstable dependency
  3. Observe how StrictMode is not able to help here
  • Incorrect code
const {propA, propB, ...rest} = propsC;

useEffect(() => {
  // some effect not expected executed on every render
}, [rest]);
// ^^ rest is "unstable"
<Component
  onSomething={() => {}}
  // we dont know what  Component will do with unstable callback. May be something, may be not
/>
  • Correct code
const {propA, propB, rest} = useMemo(() => {
  const {propA, propB, ...rest} = propsC;
  return {propA, propB, rest};
}, [propC]);

useEffect(() => {
 // some effect not expected executed on every render
}, [rest]);
// ^^ rest is "stable"
<Component
  onSomething={useCallback(() => {},[])}
/>

The current behavior

If a function is pure, running it twice does not change its behavior because a pure function produces the same result every time. However, if a function is impure (for example, it mutates the data it receives), running it twice tends to be noticeable (that’s what makes it impure!) This helps you spot and fix the bug early.

https://react.dev/reference/react/StrictMode

I personally never found this behaviour any helpful. It never helped me find a bug, especially a bug related to useEffect.

The expected behavior

I would assume that StrictMode should not try to execute useEffect twice - it should render Component twice and ensure no useEffect or useMemo is invalidated.
Ideally, it should cause full application re-render to detect memoization issues spanning across multiple components, like using children in effect dependencies or passing unstable prop to a component with useEffect as such case cannot be detected by isolated re-render.


React Forget is going to change the game rules and automatically fix the problems from above, but how one can prove it without having a corresponding testing functionality one can trust?
Unfortunately, this is something very hard to implement in the user space, simultaneously something causing incidents (performance as well as reliability) on a weekly basic

@theKashey theKashey added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Apr 24, 2024
@eps1lon
Copy link
Collaborator

eps1lon commented Apr 24, 2024

const {propA, propB, rest} = useMemo(() => {
  const {propA, propB, ...rest} = propsC;
  return {propA, propB, rest};
}, [propC]);

useEffect(() => {
 // some effect not expected executed on every render
}, [rest]);
// ^^ rest is "stable"

Keep in mind that useMemo has no semantic guarantees. React could decide at any point that it reruns the memoized function because it freed up memory used by the memoized result. The rest
value would change even though propsC didn't. That's why StrictMode also runs the function passed to useMemo twice.

it should render Component twice and ensure no useEffect or useMemo is invalidated

I think this would be interesting to try out in an experiment. Could be a PR for starters. At a glance, seems reasonable to expect effect dependencies to be stable between double render. You'd also expect the committed HTML to be stable.

@theKashey
Copy link
Contributor Author

any point that it reruns the memoized function

Not really a problem if React from time to time resets one or another branch. It is a problem if such a reset happens on every render.

You'd also expect the committed HTML to be stable.

This could be a little extension to the definition of a pure function - not only produces the same output for the same input, but also runs exactly the same effects so React can optimise run once for any number of render calls.

It is very important to have this check at the lowest level (React) as optimisations in other places may only temporarily hide the problem. See reduxjs/react-redux#2160 as an example

@theKashey
Copy link
Contributor Author

Just to keep you in touch - did a dirty spike around the problem and found two models applicable to a given task

Solution 1 - "inside"

  • it reuses StrictMode double render function (
    children = renderWithHooksAgain(
    )
  • and records dependencies of every related hook being used
  • then it compares these dependencies between sibling renders
    • ➡️ a difference means "trashed" effect

A simple solution was able to reveal a few cases where we used createRef instead of useRef as well as un-memoized callbacks being directly consumed.

It also resulted in some noise from direct refs operation in hooks like usePrevious, so I had to switch to 3 renders in a row and compare the second and third in order to minimize the number of problems to tackle.

The downsides of my solution are:

  • a parallel structure recording hook usage
  • being scoped to a single component
  • sensitive to any react code anywhere (with some problems coming not from the product, but from design system)

Solution 2 - "outside"

The second solution does not require any modification of React except overriding useEffect/useMemo hooks with trackable versions.

  • rewired useEffect to create a part of useEffect + useContext
  • used said context to cause an update to every component with useEffect
  • collected information about dep change as a signal that effects will be triggered
    • I dont need to know if effect is triggered, I need to point at the dep changed so it can be corrected

Expectations:

  • when the spy is activated first: nothing happens. Can help detect endless loops like createRef + useEffect
  • when the context update is triggered: nothing happens. Can help detect any memoization problems causing top-down invalidation.

The combination of such "small cycle" and "large cycle" has proven to be extremely effective without any need to Forget things.

Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Aug 16, 2024
@eps1lon eps1lon added Type: Discussion and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Resolution: Stale Automatically closed due to inactivity labels Aug 16, 2024
@theKashey
Copy link
Contributor Author

React Compiler removes the majority of "trashing" effects and eslint is pointing to problems. However this approach is still the simplest measure to find a problem in the code that needs fixing. Simple and powerful.

If Strict Mode is still around, then this feature should be a part of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants