In StrictMode, the useState() initializer function is called twice, but one of the results is discarded

[drcmda]

React version: 17.0.1
React reconciler: 0.26.0

Steps To Reproduce

Link to code example: https://codesandbox.io/s/r3f-contact-shadow-forked-iggyv?file=/src/index.js:308-745

let log = console.log

let id = 0
function Obj(node) {
  this.id = id++
  log('constructor', this.id)
  node.addEventListener('wheel', () => log(this.id, 'wheel'), false)
}

function App(props) {
  // The object is created twice, why does react do that, it's not documented
  const [obj] = useState(() => log('new object') || new Obj(document.body))
  return <div>{obj.id}</div>
}

ReactDOM.unstable_createRoot(document.getElementById('root')).render(<App />)

old demo

https://codesandbox.io/s/r3f-contact-shadow-forked-e44m3?file=/src/index.js

This demo creates a local object which is supposed to live within the components lifecycle.
For some reason concurrent mode creates two versions of that object, but one is stuck in the view section.
These controls aren't allowed to zoom, yet, when you give you mousewheel - it zooms. The control clearly receives the flag.

This does not happen in blocking mode and previous reconcilers (for instance react 16.4.x and reconcilers pre 0.26

Debugging in this is almost impossible as React swallows console.logs. Some users have found out that it indeed creates two branches of local state: https://twitter.com/belinburgh/status/1319990608010874883

The state object (orbit-controls) has side-effects, it creates events, but that is and should be of no consequence.

[gaearon]

Debugging in this is almost impossible as React swallows console.logs.

We're planning to make this configurable in DevTools (#19710). I agree this is pretty surprising, and unfortunately it's surprising the other way around too. For common application debugging we've found that double-logging (from double renders) is more annoying than silencing the extra render passes. But I certainly understand that the first time you discover this behavior, it's perplexing.

As a temporary workaround you can just do let log = console.log at the top level and then it'll always log.

For some reason concurrent mode creates two versions of that object,

I don't quite understand why you find this surprising. This is (and has always been) the behavior in StrictMode, too. (E.g. here's ReactDOM 16 with StrictMode running the state initializer twice.)

State initializer is part of render, and should be pure. Running it twice (and ignoring the result of one of the runs) should be completely safe, since the state initializer shouldn't mutate anything or add any listeners.

Why is running it twice a problem for you? It feels like the rest of the description assumes deep knowledge of the code so it's hard to comment on, but I can only speak to the expected behavior. Maybe you can reduce the code to a specific pattern that's breaking for you?

[gaearon]

Here is a version that logs everything (by doing the let log = ... trick): https://codesandbox.io/s/r3f-contact-shadow-forked-229sw?file=/src/OrbitControls.js

[gaearon]

The thing that immediately jumps out to me is the addEventListener inside the OrbitControls constructor. This is similar to if you called addEventListener during render. It's a side effect, which should not happen during the render phase. The fact that OrbitControls has a dispose method is a further confirmation of guess — if something needs to be "cleaned up", it clearly performs a side effect!

I think what you'd usually want to do is to split apart object creation and any side effects. E.g. new OrbitControls() should just initialize the fields, but there could be an attach() method that adds event listeners, to mirror the dispose() method. Then you call attach() from a layout effect. Effects are mirrored — so there you actually have initialization + destruction as a pair.

[drcmda]

orbitcontrols is not mine, it is a threejs primitive, i forked it for more clarity. everything has side-effects and that is something i expect react to handle simply because we don't get to decide if our dependencies are clean or not. if we pull something from npm, remark, threejs, or whatever, and we need to create a local instance (like the react docs show), it must work, or else you simply wouldn't be able to have local state in the real world. r3f for instance could close ship because all of threejs works like that.

i don't really care if it creates it twice, but it seems like it then mixes up the two that it has created. or at least we would need a way to at least dispose of the one that CM has created. this bug was introduced in react-reconciler 0.26.0. the one before is not affected in concurrent mode.

[gaearon]

If it has side effects, it needs to be done during the commit phase (eg useLayoutEffect).

[drcmda]

the problem is that in almost every case you create a local instance of something, you need it in the view. here you do it with Remark: https://reactjs.org

Whatever has changed since the previous reconciler, this pushes us into more boilerplate and indirection.

const controls = useRef()
useLayoutEffect(() => {
  controls.current = new Controls()  
}, [])
return <foo controls={controls.current} /> // doesn't exist, it would need an additional forceRefresh

or worse

const [controls, set] = useState()
useLayoutEffect(() => {
  set(new Controls())
}, [])
return controls
  ? <foo controls={controls.current} />
  : null

[gaearon]

i don't really care if it creates it twice, but it seems like it then mixes up the two that it has created

I’m not really sure what “mixes up” means here. Reducing this example to something with no third-party code would help a lot because I don’t know much about what this third-party code is supposed to be doing and I’m not familiar with the terminology. If this is truly a React bug you should be able to reproduce it with a tiny example with ReactDOM.

it must work, or else you simply wouldn't be able to have local state in the real world.

I would really appreciate if we could focus on the concrete issue and not be alarmist here. Doing a side effect during a constructor (like subscription) has always been a problem — for example, it’s completely broken if you use server rendering. I understand that R3F doesn’t, but this is not a new constraint overall for idiomatic React code. I’m sure we can figure out a solution here but a smaller self-contained example would make this much easier.

[drcmda]

before i attempt that, could you explain what it does?

i have a local state variable

const [controls] = useState(() => new OrbitControlsImpl(camera, gl.domElement))

why does concurrent mode create two controls? if i understood this a bit better i could try to make a reduced variant for react-dom. the debug log thing made it seem like magic, but now at least i know that there are to instances running. one has the zoom flag, the other --- for the strangest reason --- didn't get to the view part where enableZoom=false is set, and this i think is the bug in react.

[gaearon]

here you do it with Remark: https://reactjs.org

I want to clarify that the problem is not in creating instances. The problem is if the constructor of that instance has some side effects (such as setting up a DOM listener). This problem is not new. It is in fact the reason componentDidMount (or useLayoutEffect) exists. Separating code with side effects is what lets React support server rendering and other features in the future.

[drcmda]

i understand this part. but can you explain why one control has enableZoom=true and the other doesn't? that is the bug.

the view says: <object {...props} /> and props contain enableZoom=false.

even if CM creates two versions of orbitcontrols, one does not get to the view and the props, it is interrupted for some reason before that. that is what i need to know before i can try a react.dom version.

[gaearon]

why does concurrent mode create two controls?

This is not specific to Concurrent Mode. Again, in Strict Mode we always run the render two times in development. The second render will not "reuse" the state initializer — both of them will create one. It's just that we'll throw away the result of one of these renders in DEV.

Why do we do this? Precisely to find side effects during initialization, since that breaks other features.

[gaearon]

one does not get to the view and the props, it is interrupted for some reason before that

Concurrent Mode can't literally "interrupt" anything in your code. We're still living according to the rules of JS. If enableZoom is not what you expect, it means that something in that class's code is setting (or not setting) it. You can add a log to every place it's being set to diagnose it.

[gaearon]

I'd like to better understand the desirable behavior.

These statements are unclear to me:

one is stuck in the view section.

What do you mean by "stuck"? What is the "view section"?

one has the zoom flag, the other --- for the strangest reason --- didn't get to the view part where enableZoom=false is set

Where is that being "set"?

It would help if you could write step by step what you expected, and what is the actual behavior.