React 18 Bug: Updaters are NOT called twice for the first time when in Strict Mode

React 18 : Potential infinite loop with Suspense (and Error boundaries not triggered) ?

React version: latest stable (and experimental)

Steps To Reproduce

Checkout the following repo: https://github.com/bvaughn/react-suspense-error-boundary-bug
Run npm install && npm run dev
Load the browser and observe an infinite loop of components re-rendering (rather than the error boundary catching the error).

I've added a lot of inline comments about things that are necessary to trigger this bug in this file:
https://github.com/bvaughn/react-suspense-error-boundary-bug/blob/main/pages/index.js

Note that I was unable to reproduce this bug with Code Sandbox or Create React App. It only reproduces when running with Next JS (and only in DEV mode). Maybe it has something to do with Next's custom error logging behavior? Unfortunately there's no way to disable this (see vercel/next.js/discussions/13387) so I'm not sure.

I'm pretty sure that I know what is the scenario here. This is about React trying to render many components in "parallel" - each of them could suspend after all and I suspect that React wants to "kick-off" all of them and a single caught error (that is not a promise) doesn't abort the computation. The problem is that this particular render won't be suspending at all - all of those "parallel" components throw here. And it takes time to go through all of them, in fact - it takes more than a "concurrent deadline". So when looping over work React finally recognizes that it should yield, and it does but when going back to the work loop... it starts from scratch. So the whole work done in the previous iteration of the work loop is lost here (probably that work that has thrown errors is not cached in your React's internal structures).

This can be verified~ by decreasing the deadline from the 5ms in the scheduler package to 1ms (then an even smaller slice of those parallel components can lead to a problem). It's also easier to verify this with a console open because the JavaScript will ececute slower (probably you can even use CPU throttling, but I didn't have time to verify this last thing).

I kept digging into this. I've failed to repro this in codesandbox when I've just tried to ensure that I'm yielding in the middle of "throwing siblings" but the render was able to complete just fine. So it turns out that Brian's hunch was right here - this is, at least partially, related to Next's ReactDevOverlay and I was able to repro this on codesandbox when using this component:
https://codesandbox.io/s/thirsty-elion-nefk9t?file=/src/App.js

So I think what happens here is that:

Next catches errors that are supposed to be handled by the custom ErrorBoundary created by Brian, roughly here
it emits an event, listens to it in the ReactDevOverlay (here), and updates state there
this in turn leads to an effect being flushed and yet another state update here

So based on that I think that the tree rerenders and the partially done work is being discarded and the cycle just continues. I'm not sure if this assessment is fully correct though because ReactDevOverlay's children are always the same (its parent doesn't rerender) and IIRC this should be optimized and shouldn't require a rerender. But perhaps since the work in children didn't yet commit this doesn't happen or something.

Use <React.StrictMode>
Set a ref to a JSX element in a component
Create a useLayoutEffect/useEffect in the component where the returned cleanup function console logs the ref
Save and refresh the app
You should have access to the ref element in the the useLayoutEffect/useEffect cleanup function during the simulated unmount.

Link to code example: https://codesandbox.io/s/proud-snow-ox4ngx?file=/src/App.js

The current behavior

Does not unset/re-set refs in simulated unmount. This could lead to unexpected bugs in development like having double-set event listeners, i.e.if( ref.current ){ //add eventlistener to it }and might not match the behavior of actually unmounting the DOM node as described in the docs: https://reactjs.org/docs/strict-mode.html

On the second mount, React will restore the state from the first mount. This feature simulates user behavior such as a user tabbing away from a screen and back, ensuring that code will properly handle state restoration.

The expected behavior

In normal unmounts and mounts refs are unset(before layout effects are cleaned up) and set(before layout effects).

I confirm. In my case, this causes a nasty bug. Everything related refs that worked in version 17 just stopped working in version 18 without any error messages. I definitely can't use React.StrictMode right now.

Moreover, during the first installation using create-react-app, a person will not be able to use any library that uses the refs functionality, thinking that the library does not work. My suggestion is to remove React.StrictMode from default create-react-app set up until the bug is fixed.

According to this post:

Whether a component gets hidden or unmounted, refs that are attached to host components (like HTML elements) will be cleaned up and re-initialized the same as they are today during unmount/remount.

Refs that store use-managed values won't be reset when a component is hidden though. They allow things to be persisted for the case where a component is hidden and later shown again.

It makes sense to keep the user-managed values around as they are part of the component state that will be restored, however refs to DOM elements maybe should not be set?

Please check this package https://github.com/oney/kill-use-callback

depFn is not a React hook that needs to depend on React lifecycle, so depFn can be used anywhere like in a condition or in a loop.

depFn makes it possible to optimize any callbacks in compiler(e.g. babel) by automatically collecting all values to deps array. Therefore, developers don't have to write useCallback by themselves anymore, and we can totally remove useCallback hook in React core.

The key concept of depFn is:

If two closures have the same "function body code", and the same "environment" (or context) which actually means the dependency of values or references, these two closures can be considered as equal

Don't forget this in the doc:

In the future, a sufficiently advanced compiler could create this array automatically.

To be clear, what I mean is when developers write the code like

function App({items, text}) {
  const getText = text.length === 10 ? undefined
    : (prefix) => `${prefix}: ${text}`;
  useEffect(() => { /* ... */ }, [getText]);

  return (<div>{items.map(item => (
    <Child getText={() => `${item}: ${text}`} />
  ))}</div>);
}

The compiler collects deps array and wraps callback with depFn automatically:

function App({items, text}) {
  const getText = text.length === 10 ? undefined
    : depFn((prefix) => `${prefix}: ${text}`, [text]);
  useEffect(() => { /* ... */ }, [getText]);

  return (<div>{items.map(item => (
    <Child getText={depFn(() => `${item}: ${text}`, [text])} />
  ))}</div>);
}

Since React 18, useEffect is called twice in Strict Mode when zero dependencies.

This is following on from this tweet:
https://twitter.com/dan_abramov/status/1523652274748559360

The purpose of logging this issue is not to report the problem. The purpose of this issue is to present a scenario where the new behaviour is awkward / difficult to work with, migrating components is not easy, as such I advocate calling this out as a breaking change, and hopefully something that can be turned off in future React versions.

Link to code example:

https://stackblitz.com/edit/react-useeffect-called-twice-scenario

React version: 18

Steps To Reproduce:

Note the UI rendering with / without strict mode is different.

** Problem Pattern 1

UI components from libraries need to maintain their own state & services. They cannot participate with state & services of the hosting application, as then encapsulation of the component is lost. A reliable way is needed to create the state & services of such a component bound to the lifecycle of the component. The new pattern (useEffect() called twice) means the state & services will unnecessarily get created and destroyed twice, along with the useEffect. For a datagrid, this could mean 100,000 rows passed to the grid getting sorted and grouped twice, when it should be once. This has two bad outcomes 1) consumers of the library will observe this and will complain, without knowing it's an intended pattern of React 2) it is bad practice to have different code paths executed in Dev vs Prod modes.

** Problem Pattern 2

For groups of components, where there is shared logic that executes when all components are ready, there is no reliable way to know when all required components are ready, as the components can get destroyed and re-initialised.

In the provided example, note that each React Component works with a Controller class. All the logic is delegated to the Controller class, making the React Component only responsible for DOM operations. This allows the UI to be swapped out with a different rendering engine, while keeping all the logic. This is what AG Grid uses to allow it to use React to render when used with React, and SomethingElse when used with SomethingElse. There are parts of the logic that wait for all Controllers (and associated Components) to be ready before doing certain steps. In React, because the Controllers lifecycle is tied to the useEffect, it means the Controllers are not tied to the Components lifecycle, and resulting in stale references for old Controllers.

In the example code, see ControllersService. This service is what the components register to, and when all components are registered, grid code that was waiting for the components to be ready is notified.

** Breaking Change

This change forces the other non-UI parts of the application to have a fundamental change in how they work. This means React is breaking from the following statement (taken from reactjs.org): "Learn Once, Write Anywhere - We don’t make assumptions about the rest of your technology stack, so you can develop new features in React without rewriting existing code."

Yes we could re-write how we do things in AG Grid, however then the bug I am raising is this needs to be documented as a breaking change and React 18 is not backwards compatible.

this needs to be documented as a breaking change and React 18 is not backwards compatible.

Just to start with this — React 18 is a major release and is not meant to be fully backwards-compatible. Each major release has breaking changes; otherwise we wouldn't mark them as major. The change is documented in the upgrade blog post. It is also marked as a breaking change in the changelog.

I'll respond to the rest later.

Just to start with this — React 18 is a major release and is not meant to be fully backwards-compatible. Each major release has breaking changes; otherwise we wouldn't mark them as major. The change is documented in the upgrade blog post. It is also marked as a breaking change in the changelog.

Noted, fair points.

My workaround:

import { DependencyList, EffectCallback, useEffect } from 'react'

export default process.env.NODE_ENV === 'production'
	? useEffect
	: (effect: EffectCallback, deps?: DependencyList) => {
			useEffect(() => {
				let ready = true
				let destructor: void | (() => void)

				queueMicrotask(() => {
					if (ready) destructor = effect()
				})

				return () => {
					ready = false
					destructor?.()
				}
			}, deps)
	  }