react-three-fiber
react-three-fiber copied to clipboard
pmndrs
Primitive with children mounting / unmounting + documentation
The documentation says the following about primitives
You can still give it properties or attach nodes to it. [...] Primitives will not dispose of the object they carry on unmount, you are responsible for disposing of it!
And in the source code it says
Primitives should not have objects and children that are attached to them declaratively ...
The code comment seems to be clear about how primitives shouldn't have any children declaratively while the documentation says we can add nodes to it.
It might be me misunderstanding but they seem to contradict each other and it seems that adding children to primitives isn't really supported.
I made some test to check it out.
Unmount primitive parent: expect children to be removed from the scene ✔ https://codesandbox.io/p/sandbox/elegant-kepler-grc767?file=%2Fsrc%2FApp.js%3A39%2C31 Change primitive key: expect children to be removed from the scene before being re-created or reused❌( they're recreated and the previous are left in the scene ) https://codesandbox.io/p/sandbox/musing-bush-86gyj7?file=%2Fsrc%2FApp.js%3A36%2C13 Change group key: expect children to be removed from the scene before being re-created or reused ✔ https://codesandbox.io/p/sandbox/floral-wave-5mj74v?file=%2Fsrc%2FApp.js%3A38%2C30
It seems that either some primitive case aren't handled correctly yet or that the documentation should state that no children should be passed to primitives.
The source code is outdated and you can write JSX beneath <primitive />. We've been improving on no.2 specifically and can't get it completely right until v9 which brings architectural changes since this is an inherent issue with React's reconciliation. We're sensitive to the order of operations, and needed a way of separating destructive actions from it -- #2250 being the prime motivation. No. 2 can be sidestepped with a stable key or UUID.
cc @krispya I don't see a PR associated with this being closed. What was the resolution for this issue?
Thanks for you work!
cc @CodyJasonBennett
The v9 reconciler updates solve this case. I think all I can do to associate a PR is add some tests.
Gah... found a bug adding the tests. That's what I get for moving too fast.