Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v9] fix: primitive children mounting #3248

Merged
merged 6 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 18 additions & 3 deletions packages/fiber/src/core/reconciler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,12 @@ export const extend = <T extends Catalogue | ConstructorRepresentation>(
}
}

function createInstance(type: string, props: HostConfig['props'], root: RootStore): HostConfig['instance'] {
function createInstance(
type: string,
props: HostConfig['props'],
root: RootStore,
flushPrimitive = true,
): HostConfig['instance'] {
// Get target from catalogue
const name = `${type[0].toUpperCase()}${type.slice(1)}`
const target = catalogue[name]
Expand All @@ -121,6 +126,9 @@ function createInstance(type: string, props: HostConfig['props'], root: RootStor
// Throw if an object or literal was passed for args
if (props.args !== undefined && !Array.isArray(props.args)) throw new Error('R3F: The args prop must be an array!')

// Regenerate the R3F instance for primitives to simulate a new object
if (flushPrimitive && type === 'primitive' && props.object?.__r3f) delete props.object.__r3f

// Create instance
const instance = prepare(props.object, root, type, props)

Expand Down Expand Up @@ -309,7 +317,12 @@ function switchInstance(
if (oldInstance.isHidden) unhideInstance(oldInstance)

// Create a new instance
const newInstance = createInstance(type, props, oldInstance.root)
const newInstance = createInstance(type, props, oldInstance.root, false)

// Update attach props for primitives since we don't flush them
if (type === 'primitive') {
newInstance.props.attach = props.attach
}
Comment on lines +322 to +325
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably an issue with applyProps not being called for primitives in prepare or the reference being broken somewhere.


// Move children to new instance
for (const child of oldInstance.children) {
Expand Down Expand Up @@ -377,7 +390,9 @@ export const reconciler = Reconciler<
supportsMutation: true,
supportsPersistence: false,
supportsHydration: false,
createInstance,
createInstance(type, props, root) {
return createInstance(type, props, root)
},
removeChild,
appendChild,
appendInitialChild: appendChild,
Expand Down
67 changes: 64 additions & 3 deletions packages/fiber/tests/renderer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,69 @@ describe('renderer', () => {
expect(object.name).toBe('primitive')
})

it('should remove children from primitive when unmounted', async () => {
const object = new THREE.Group()

function Parent({ children, show }: { children: React.ReactNode; show: boolean }) {
return show ? <primitive object={object}>{children}</primitive> : null
}

function Component({ show }: { show: boolean }) {
return (
<Parent show={show}>
<group name="A" />
<group name="B" />
</Parent>
)
}

const store = await act(async () => root.render(<Component show={true} />))
const { scene } = store.getState()

expect(scene.children.length).toBe(1)
expect(scene.children[0]).toBe(object)
expect(object.children.length).toBe(2)

await act(async () => root.render(<Component show={false} />))

expect(scene.children.length).toBe(0)
expect(object.children.length).toBe(0)
})

it('should remove then add children from primitive when key changes', async () => {
const object = new THREE.Group()

function Parent({ children, primitiveKey }: { children: React.ReactNode; primitiveKey: string }) {
return (
<primitive key={primitiveKey} object={object}>
{children}
</primitive>
)
}

function Component({ primitiveKey }: { primitiveKey: string }) {
return (
<Parent primitiveKey={primitiveKey}>
<group name="A" />
<group name="B" />
</Parent>
)
}

const store = await act(async () => root.render(<Component primitiveKey="A" />))
const { scene } = store.getState()

expect(scene.children.length).toBe(1)
expect(scene.children[0]).toBe(object)
expect(object.children.length).toBe(2)

await act(async () => root.render(<Component primitiveKey="B" />))

expect(scene.children.length).toBe(1)
expect(scene.children[0]).toBe(object)
expect(object.children.length).toBe(2)
})

it('should go through lifecycle', async () => {
const lifecycle: string[] = []

Expand Down Expand Up @@ -444,11 +507,9 @@ describe('renderer', () => {
expect(scene.children.map((o) => o.name)).toStrictEqual(mixedArray.map((o) => o.name))
})

// TODO: fix this case, also see:
// https://github.com/pmndrs/react-three-fiber/issues/1892
// https://github.com/pmndrs/react-three-fiber/issues/3125
// https://github.com/pmndrs/react-three-fiber/issues/3143
it.skip('can swap 4 array primitives via attach', async () => {
it('can swap 4 array primitives via attach', async () => {
const a = new THREE.Group()
a.name = 'a'
const b = new THREE.Group()
Expand Down