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

React 18: useEffect different behaviour compare to v17 in specific cases #23090

Open
dmitryKochergin opened this issue Jan 11, 2022 · 10 comments
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion

Comments

@dmitryKochergin
Copy link

Hi!
In this issue to react-query repo we found some strange useEffect behaviour.

Here's examples:
react 17
react 18 with old root api

Just click "rerender" button and see that there's no effect firing.

But in react 18 with the new root API it works fine:
codesandbox

I tried to explain it like that:

So, in our case we have dummy and forceRerender state variables.
After calling rerender function, dummy updates with the same value, and React bails out without rendering the children or firing effects. I suppose that somewhere here the effect should've been scheduled, but it didn't because of that react behaviour.
BUT, the render function was actually called and useEffect received updated value in deps (you can see this by checking how many times console.log was called in render).
And then on the next tick forceRerender causes another update, but this time it doesn't schedule the effect because it has the same deps (value) that were captured in the previous render function call.

So, is this right explanation for what is going on there?
And I guess, the main question, why does this example work fine when the new root api is used?
Does it mean that with the new root api (and with concurrent mode) useEffect compare deps only with those from the last previous render that was committed to the DOM?

Thanks!

@dmitryKochergin dmitryKochergin added React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion labels Jan 11, 2022
@vkurchatkin
Copy link

It's an interesting case, but the thing is, the code is definitely incorrect, you can't use external mutable values as useEffect dependencies (or at all in render). Everything that follows can be qualified as "undefined behaviour".

@dmitryKochergin
Copy link
Author

@vkurchatkin Hi!
Generally speaking, yes. But I can't fully agree with you. A lot of modern libraries for react use approach, when they have some internal storage, and their own subscription model for updating it. And storage indeed isn't a part of react state there, for updating they use subsription handlers like forceRerender(i => i + 1).

My example could be too simple, here's almost the same example, but with react-query:
codesandbox

react-query do so, and react-redux did before v7 (or about).
In react 18 we will have useSyncExternalStore for this.
But before it, using external mutable values like "state" was the only way to connect framework agnostic code/library with react.
At least I guess so 😄

@bitshiftnetau

This comment has been minimized.

@lgenzelis
Copy link

@vkurchatkin yes, the simplified code itself is incorrect, but I feel extremely curious as to why this happens. I mean, the component is re-rendering, and the value of the deps array for useEffect is different. Then, why is it not firing? 🤯 😖

@eps1lon
Copy link
Collaborator

eps1lon commented Apr 29, 2022

The effect is firing with 18.1: https://codesandbox.io/s/react-bug-version-3-forked-dofmh8?file=/src/example.js

@dmitryKochergin Could you confirm that this is now fixed?

@dmitryKochergin
Copy link
Author

@eps1lon Yes, with the new root API it works fine.
With the old root API the bug still occurs, though.
Since the old root API is unsupported we could probably close this issue.

@eps1lon
Copy link
Collaborator

eps1lon commented May 4, 2022

With the old root API the bug still occurs, though.

I don't understand what the bug with the old root API is? Could you point me again to a CodeSandbox with the latest version of React and the old root API and explain what you see and what you expected instead?

@lgenzelis
Copy link

@eps1lon here's a codesandbox with react 18.1 which still shows the bug. Bug: useEffect doesn't run when the component re-renders, even though its dependency array ([value]) changes. You need to open the console to actually see the console.log coming from useEffect not being printed.

If you go to index.js and change

ReactDOM.render(
  <StrictMode>
    <Example />
  </StrictMode>,
  rootElement
);

by

const root = ReactDOM.createRoot(rootElement);
root.render(<StrictMode><Example /></StrictMode>);

you'll see the expected behavior (that is, useEffect gets triggered for each re-render).

@blixt
Copy link

blixt commented Oct 24, 2022

I've encountered this behavior change as well. I'm not sure how to create a reproduction case out of our code base, but essentially what I see is that given the following logic:

const value = useCustomHook()
console.log("value is now:", value)
useEffect(() => {
    console.log("effect is running for value:", value)
}, [value])

Then the following logs can be observed:

value is now: 0
effect is running for value: 0
value is now: 1
value is now: 2
…

Essentially, even if the component is clearly re-rendering multiple times (the re-render is triggered via a useReducer dispatch inside of the custom hook after which it returns the new value), the effect never gets rerun. The value in the deps array is a primitive (string in our case) and is definitely changing between renders (I logged the exact copy/paste of the array on the line before the call to useEffect and the immediate values inside the array do change so it would be different in a shallow compare).

It's almost as if the first render where the deps change doesn't trigger the useEffect but does make it remember the new deps values, then next time around when the useEffect would've triggered, it's now no longer different…

If the repro cases in this issue aren't enough, I can take a stab at creating one from our code base but my first attempt did not reproduce this bug.

This behavior is observed using either API to mount React.

@blixt
Copy link

blixt commented Oct 24, 2022

Okay so I did end up managing to reproduce this. Basically if you synchronously end up in a flow like the following, a component will re-render with new state, but not trigger effects, which I think is a bug:

  1. Parent class component is being re-rendered
  2. Inside shouldComponentUpdate of the parent , an external store function is called that triggers its update listeners to be called
  3. A child component (note: not sure if it has to be a child component) is re-rendered because it's listening to the external store
  4. Meanwhile, another class component in between parent and child will return false in shouldComponentUpdate (the parent will return true)
  5. The end result is a re-rendered child component with the new value, but its useEffect depending on that value never ran

Here's the small repro: https://codesandbox.io/s/funny-galois-1wr2ib?file=/src/App.js

To test the repro you'd follow these steps:

  1. Click "Update store" one or more times to (invisibly) queue up a value change in the external store without causing React to do anything
  2. Click "Flush changes" to make the external store call the listeners that are interested in changes
  3. Observe that the component re-renders and logs the new value, but its useEffect doesn't log anything (it neither cleans up the old effect, nor does it initialize a new one)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion
Projects
None yet
Development

No branches or pull requests

6 participants