pmndrs / react-three-fiber

🇨🇭 A React renderer for Three.js
https://docs.pmnd.rs/react-three-fiber
MIT License
26.28k stars 1.49k forks source link

Primitive with children mounting / unmounting + documentation #3204

Open AlaricBaraou opened 2 months ago

AlaricBaraou commented 2 months ago

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.

CodyJasonBennett commented 2 months ago

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.

gkjohnson commented 4 weeks ago

cc @krispya I don't see a PR associated with this being closed. What was the resolution for this issue?

Thanks for you work!

krispya commented 4 weeks ago

cc @CodyJasonBennett

The v9 reconciler updates solve this case. I think all I can do to associate a PR is add some tests.

krispya commented 4 weeks ago

Gah... found a bug adding the tests. That's what I get for moving too fast.