fix(applyProps): harden copy case

pmndrs · Feb 17, 2025 · f0edb94 · f0edb94
1 parent e95baa8
commit f0edb94
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 97 deletions.
diff --git a/packages/fiber/src/core/utils.tsx b/packages/fiber/src/core/utils.tsx
@@ -401,34 +401,30 @@ export function applyProps<T = any>(object: Instance<T>['object'], props: Instan
 
     let { root, key, target } = resolve(object, prop)
 
-    // Copy if properties match signatures
-    if (
-      target?.copy &&
-      (value as ClassConstructor | undefined)?.constructor &&
+    // Layers must be written to the mask property
+    if (target instanceof THREE.Layers && value instanceof THREE.Layers) {
+      target.mask = value.mask
+    }
+    // Copy if properties match signatures and implement math interface (likely read-only)
+    else if (
+      target &&
+      typeof target.set === 'function' &&
+      typeof target.copy === 'function' &&
+      (value as ClassConstructor | null)?.constructor &&
       (target as ClassConstructor).constructor === (value as ClassConstructor).constructor
     ) {
-      // fetch the default state of the target
-      const ctor = getMemoizedPrototype(root)
-      // The target key was originally null or undefined, which indicates that the object which
-      // is now present was externally set by the user, we should therefore assign the value directly
-      if (!is.und(ctor) && (is.und(ctor[key]) || is.nul(ctor[key]))) root[key] = value
-      // Otherwise copy is correct
-      else target.copy(value)
-    }
-    // Layers have no copy function, we must therefore copy the mask property
-    else if (target instanceof THREE.Layers && value instanceof THREE.Layers) {
-      target.mask = value.mask
+      target.copy(value)
     }
     // Set array types
-    else if (target?.set && Array.isArray(value)) {
-      if (target.fromArray) target.fromArray(value)
+    else if (target && typeof target.set === 'function' && Array.isArray(value)) {
+      if (typeof target.fromArray === 'function') target.fromArray(value)
       else target.set(...value)
     }
     // Set literal types
-    else if (target?.set && typeof value !== 'object') {
-      const isColor = (target as unknown as THREE.Color | undefined)?.isColor
+    else if (target && typeof value === 'number') {
+      const isColor = (target as unknown as THREE.Color)?.isColor
       // Allow setting array scalars
-      if (!isColor && target.setScalar && typeof value === 'number') target.setScalar(value)
+      if (!isColor && typeof target.setScalar === 'function' && typeof value === 'number') target.setScalar(value)
       // Otherwise just set single value
       else target.set(value)
     }

diff --git a/packages/fiber/tests/utils.test.ts b/packages/fiber/tests/utils.test.ts
@@ -346,72 +346,41 @@ describe('applyProps', () => {
     expect(target.foo.value).toBe(false)
   })
 
-  it('should prefer to copy from external props', () => {
-    const copyMock = jest.fn()
-
-    class MockedColor extends THREE.Color {
-      override copy(other: MockedColor) {
-        copyMock()
-        return super.copy(other)
-      }
-    }
-
-    class MockedClass {
-      color = new MockedColor()
-      layer = new THREE.Layers()
-    }
-
-    const target = new MockedClass()
+  it('should prefer to copy potentially read-only math classes', () => {
+    const one = new THREE.Vector3(1, 1, 1)
+    const two = new THREE.Vector3(2, 2, 2)
 
-    // Same constructor, copy
-    applyProps(target, { color: new MockedColor() })
-    expect(target.color).toBeInstanceOf(MockedColor)
-    expect(copyMock).toHaveBeenCalledTimes(1)
+    const target = { test: one }
+    applyProps(target, { test: two })
 
-    // Same constructor, Layers
-    const layer = new THREE.Layers()
-    layer.mask = 5
-    applyProps(target, { layer })
-    expect(target.layer).toBeInstanceOf(THREE.Layers)
-    expect(target.layer.mask).toBe(layer.mask)
-
-    // Different constructor, overwrite
-    applyProps(target, { color: new THREE.Vector3() })
-    expect(target.color).toBeInstanceOf(THREE.Vector3)
-    expect(copyMock).toHaveBeenCalledTimes(1)
+    expect(target.test).toBe(one)
+    expect(target.test.toArray()).toStrictEqual([2, 2, 2])
   })
 
-  it('should prefer to assign if origin null', () => {
-    const copyMock = jest.fn()
+  it('should not copy if props are supersets of another', () => {
+    const copy = jest.fn()
+    const set = jest.fn()
 
-    class MockedTexture extends THREE.Texture {
-      override copy(other: MockedTexture) {
-        copyMock()
-        return super.copy(other)
-      }
+    class Test {
+      copy = copy
+      set = set
     }
-
-    class MockedClass {
-      map1 = null
-      map2 = new MockedTexture()
+    class SuperTest extends Test {
+      copy = copy
+      set = set
     }
 
-    const target = new MockedClass()
+    const one = new Test()
+    const two = new SuperTest()
 
-    // Original null, should assign
-    applyProps(target, { map1: new MockedTexture() })
-    expect(target.map1).toBeInstanceOf(MockedTexture)
-    expect(copyMock).toHaveBeenCalledTimes(0)
+    const target = { test: one }
+    applyProps(target, { test: two })
 
-    // Set again, original null, should assign
-    applyProps(target, { map1: new MockedTexture() })
-    expect(target.map1).toBeInstanceOf(MockedTexture)
-    expect(copyMock).toHaveBeenCalledTimes(0)
-
-    // Original not null, should copy
-    applyProps(target, { map2: new MockedTexture() })
-    expect(target.map2).toBeInstanceOf(MockedTexture)
-    expect(copyMock).toHaveBeenCalledTimes(1)
+    expect(one.copy).not.toHaveBeenCalled()
+    expect(two.copy).not.toHaveBeenCalled()
+    expect(one.set).not.toHaveBeenCalled()
+    expect(two.set).not.toHaveBeenCalled()
+    expect(target.test).toBe(two)
   })
 
   it('should prefer to set when props are an array', () => {
@@ -458,27 +427,6 @@ describe('applyProps', () => {
 
     expect('onClick' in target).toBe(false)
   })
-
-  it('should not copy if props are supersets of another', () => {
-    const copy = jest.fn()
-
-    class Material {
-      copy = copy
-    }
-    class SuperMaterial extends Material {
-      copy = copy
-    }
-
-    const one = new Material()
-    const two = new SuperMaterial()
-
-    const target = { material: one }
-    applyProps(target, { material: two })
-
-    expect(one.copy).not.toHaveBeenCalled()
-    expect(two.copy).not.toHaveBeenCalled()
-    expect(target.material).toBe(two)
-  })
 })
 
 describe('updateCamera', () => {