React: [reconciler] re-instanciating object in commitUpdate

[drcmda]

Do you want to request a feature or report a bug?

feature, this is for https://github.com/drcmda/react-three-fiber

What is the current behavior?

commitUpdate is supposed to simply update the instance, it seems that it cannot "exchange" the instance.

What is the expected behavior?

I need to re-instanciate the instance, just writing props to it is not enough in my case. Basically i am trying this:

  commitUpdate(instance, updatePayload, type, oldProps, newProps, fiber) {
    if (instance.isObject3D) {
      applyProps(instance, newProps, oldProps)
    } else {
      // This is a data object, let's extract critical information about it
      const parent = instance.__parent
      const { args: argsNew = [], ...restNew } = newProps
      const { args: argsOld = [], ...restOld } = oldProps
      // If it has new props or arguments, then it needs to be re-instanciated
      if (argsNew.some((value, index) => value !== argsOld[index])) {
        // First it gets removed from its parent
        removeChild(parent, instance)
        // Next we create a new instance and append it again
        const newInstance = createInstance(instance.type, newProps)
        appendChild(parent, newInstance)
        // Switch fiber node (???)
        fiber.stateNode = newInstance // <---- this doesn't work

This is a THREE data object which i am trying to map. Sometimes these objects cannot be mutated, THREE expects them to be re-created. One example of this is: https://threejs.org/docs/index.html#api/en/geometries/PlaneGeometry

Any modification after instantiation does not change the geometry.

I am creating the object in JSX like so:

<planeGeometry name="geometry" args={[width, height]} />

When the args change the instance has to be recreated, no other way around unfortunately.

It would be amazing if commitUpdate would be allowed to return an instance. And if it does the reconciler writes it into the fiber node. I am trying fiber.stateNode here, but it doesn't help. Next time commitUpdate is called it passes on the old instance again, which doesn't have a parent any longer.

I don't really understand how these nodes work but the stacktrace says

commitWork(_current2, nextEffect)

where nextEffect ends up being "fiber" in the commitUpdate. "_current2" seems to be the one that still clings to the old stateNode.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.8.3

[drcmda]

Oh, i just tried something that did actually work:

        // Switch fiber node
        fiber.alternate.stateNode = newInstance
        fiber.stateNode = newInstance

could i get a confirmation that this can be a viable way forward?

[gaearon]

No, changing fibers is not supported and can break things very badly.

You can, however, make your “instance” a plain object that contains a Three node. Then you can do anything with it, including recreating it when necessary.

[drcmda]

it wouldn't change the ref, though, just the stateNode, but you're completely right, i shouldn't mess with fiber. do you mean doing it like a ref.current? that could actually work. i never see this stuff ... 🤯 thanks again dan!

[gaearon]

No, I mean that your “instance” you create in createInstance doesn’t literally have to be a Three instance. It can be a wrapper object like { threeNode: ... }. This lets you replace it later in commitUpdate if needed.

[drcmda]

that worked perfectly!

[drcmda]

@gaearon this has now created another problem, users fetching refs on these objects now get served the wrapper while for native objects they get served the real object:

<mesh ref={mesh}>
  <geometry ref={geo} />

mesh will point to the actual object in the scene (mesh.current === THREE.Mesh)

geo is a random class that could get re-instanciated in some cases. It will point to the wrapper (geo.current.object === THEE.Geometry)

This is really confusing. Is there anything in reconciler that allows me to get in between react and the end-user for served refs? Something like getRef(instance, ref) { return ref } If not, can it please be made?

Otherwise the api isn't gonna work. I would be forced to wrap all objects just for consistency, which adds more overhead.

[gaearon]

That’s what getPublicInstance host config method should let you do.

getPublicInstance(node) {
  return node.threeNode;
}

[gaearon]

Although I guess that wouldn’t update it. ☹️

[gaearon]

I guess the canonical solution in your case is wrapper components. Like in React Native.

const PlaneGeometry = React.forwardRef((props, ref) => {
  const key = props.args.width + ':' + props.args.height;
  return <planeGeometry {...props} key={key} ref={ref} />;
});

This lets us give it a key which will force remounting.

Then you don’t need wrappers internally and your refs are clean. But some components would have to be imported.

import { PlaneGeometry } from 'package'

The upside of this is it lets you add some nicer API on top if you want without complicating your reconciler config.

If you go this way it would probably make sense to do for all primitives and not just special ones.

[drcmda]

@gaearon That is what i would like to avoid, because THREE has hundreds of objects i would need to ship and maintain. They're constantly changing and shifting stuff. Doing it on heuristics and with keynames allows me to move fast and be mostly independent of THREE. Treeshaking is another concern.

I've tried getPublicInstance and you're right, it works, but then doesn't update.

  getPublicInstance(instance) {
    return instance.isObject3D ? instance : instance.obj
  },

  ...

  const [vec, setVec] = useState([1,2,3])
  const vecRef = useRef()
  useEffect(() => void setTimeout(() => setVec([4,5,6]), 4000), [])
  useEffect(() => void console.log("REF ->", vecRef), [vec])

  return <vector3 ref={vecRef} args={vec} />

vecRef.current will be [1,2,3] two times ...

A solution would be a new reconciler-method that gets called in between ref passing, or something in commitUpdate that would allow switching nodes. I could imagine i won't be the last person bumping into this. Is it realistic that this could be considered?

[gaearon]

So how many of those objects are weird like this and need to be recreated?

[gaearon]

Also you don’t literally have to define them all. You can generate them by walking over Three exports. And only write the ones that have manual logic.

[drcmda]

It's not just the ones that three provides (which are many, + the examples folder which contains shaders, effects, etc), all stuff i wouldn't like to touch normally because it creates references that will kill tree shaking. But worse, you can make your own arbitrary ones and it's very normal in the three eco system to extend existing classes and so on. it's kind of the wild west and the lib comes from the IIEF time. Cherry on top, with the <primitive object={...} /> element i basically allow them to dynamically include any object.

[gaearon]

Sorry, I don't see how treeshaking fits into this.

I'm saying you can do

Object.keys(THREE).forEach(key => {
  YourLib[key] = createWrapperComponent(key);
})

Not literally that but it should give you an idea. I don't see how tree shaking is relevant here. Alternatively, you could export a Proxy that lazily creates component wrappers when its properties are accessed. If this doesn't work, please provide an example why.

[drcmda]

a proxy seems like way too much overhead for something that simple tbh. i just wanna exchange an instance. 😔exporting all objects as wrapped classes doesn't seem like the way it should go for me, wouldn't wanna let go of the comfort and ease of native elements.

right now i am thinking, i could mutate the fiber instance and live dangerous. or force them to do ref.current.object - both seem better than proxies and object exports.

[gaearon]

What overhead do you mean? It's something like three lines of code:

function createWrapper(name) {
  return 'wrapper for ' + name;
}

let Lib = new Proxy({}, {
  get(target, prop, receiver) {
    return createWrapper(prop);
  }
});

console.log(Lib.geometry) // 'wrapper for geometry'
console.log(Lib.bazinga) // 'wrapper for bazinga'

[gaearon]

Maybe you misunderstood. I didn't mean a Proxy for the instances. I meant a Proxy for exports. So that people can write <YourLib.ArbitraryName> and get a type representing 'arbitraryName'.

[drcmda]

wouldn't that exclude ie11, or warrant polyfills (if they even exist, i don't know)?

[drcmda]

I mean, re-instantiating is not so uncommon, It could be something that reconciler supports with a mapping function ref = reconcilerConfig.mapRef ? reconcilerConfig.mapRef(ref) : ref and it's solved for all. three won't be the only platform that needs it.

[gaearon]

wouldn't that exclude ie11, or warrant polyfills (if they even exist, i don't know)?

Does THREE work there? I'm not very familiar. Yes, proxies wouldn't work in IE but I assumed you're targeting modern browsers.

three won't be the only platform that needs it.

So far all other platforms were happy with exporting wrapping components that contain extra logic like this. This is the first time this problem comes up which is why I'm pushing back.

It could be something that reconciler supports with a tiny mapping function

It's rarely "tiny" when we're talking about changing the reconciler. :-) Changes like this have downstream effects. It's not obvious to me that it would be a simple change but I'm not sure. If you send a PR implementing it we can think about it.

[drcmda]

yes, three works in ie11. the reconciler is a company thing i am making for a tooling application, but doing it open source so that others can benefit too, but at least in my case i can't shut out ie, they'd just terminate the project.

If you send a PR implementing it we can think about it.

I will study the source a little and see if i can understand it good enough.

which is why I'm pushing back.

Appreciate the discussion and time you sacrifice, i totally understand your caution.

[gaearon]

How many of those special objects are there?

You could allow to normally use <thing /> but when a special one is used, warn to use imported <Thing /> instead.

[drcmda]

ran a map over the default exports, there are 48 Object3D's and 363 raw objects that could need re-instantiation. but that's just the default export, would be a whole bunch more when traversing the extras.

going through the react source right now and i see now that just mapping the ref isn't going to work, commitAttachRef is only called once. now changing stateNode in the fiber after commitUpdate could be a way, but is there any explanation why there are always two, fiber and fiber.alternate. is stateNode stored anywhere else or can it be safely exchanged inside the reconciler as a new kind of rule that allows instance-exchange?

[gaearon]

ran a map over the default exports, there are 48 Object3D's and 363 raw objects that could need re-instantiation. but that's just the default export, would be a whole bunch more when traversing the extras.

Looping over 300 items once is very fast. You don't have to actually instantiate the wrappers — you can define getters which do that lazily.

[sompylasar]

@gaearon THREE library components and modules are not uniformly structured and exported, so the recurring effort is not in the loop itself but in the maintenance of that loop.

[gaearon]

Can you always do something like this then? <node type={THREE.Thing} />

[drcmda]

I have thought about it, but decided against it in the beginning because it thrashes the syntax a bit. To put a clean <geometry /> under a <mesh> would be quite the nice experience. Also helps reducing cognitive overhead because it's the same api, the difference (and problem) stems from three itself internally throwing wrenches, would be nice to abstract that away with the reconciler.

[gaearon]

I think I stand by saying the canonical solution is to provide wrapping components for the ones that need special logic like this. You're free to try other things (like patching stateNode or sending a PR) but all guarantees are off and you might find gnarly unfixable bugs later.

[drcmda]

That's alright with me. i'm trying a PR and let's see if something comes out of it. If not, it's not the end. Thanks again! : )

[gaearon]

I’m curious what was the outcome of this. Have you kept the stateNode hack, or did you find another solution?