fix(core): fix props proxy target to avoid double $effect runs (#494)

Fixes #493

Author: mcous

jest.config.js

@@ -6,7 +6,7 @@ const SVELTE_TRANSFORM_PATTERN =
     : String.raw`^.+\.svelte$`
 
 export default {
-  testMatch: ['<rootDir>/tests/**/*.test.js'],
+  testMatch: ['<rootDir>/tests/**/*.test.{js,svelte.js}'],
   transform: {
     [SVELTE_TRANSFORM_PATTERN]: 'svelte-jester',
   },

packages/svelte-core/src/props.svelte.js

@@ -1,8 +1,8 @@
 /**
  * Create a shallowly reactive props object.
  *
- * This allows us to update props on `rerender`
- * without turing `props` into a deep set of Proxy objects
+ * This allows us to update props on `rerender` without turning the user's
+ * props into a deeply reactive `$state` proxy.
  *
  * @template {Record<string, unknown>} Props
  * @param {Props} initialProps
@@ -11,21 +11,26 @@
 const createProps = (initialProps = {}) => {
   let currentProps = $state.raw(initialProps)
 
-  const props = new Proxy(initialProps, {
-    get(_, key) {
-      return currentProps[key]
-    },
-    set(_, key, value) {
-      currentProps[key] = value
-      return true
-    },
-    has(_, key) {
-      return Reflect.has(currentProps, key)
-    },
-    ownKeys() {
-      return Reflect.ownKeys(currentProps)
-    },
-  })
+  const props = new Proxy(
+    {},
+    {
+      get(_, key) {
+        return Reflect.get(currentProps, key)
+      },
+      set(_, key, value) {
+        return Reflect.set(currentProps, key, value)
+      },
+      has(_, key) {
+        return Reflect.has(currentProps, key)
+      },
+      ownKeys() {
+        return Reflect.ownKeys(currentProps)
+      },
+      getOwnPropertyDescriptor(_, key) {
+        return Reflect.getOwnPropertyDescriptor(currentProps, key)
+      },
+    }
+  )
 
   const update = (nextProps) => {
     currentProps = { ...currentProps, ...nextProps }

tests/bind.test.svelte.js

@@ -0,0 +1,77 @@
+import { render, screen } from '@testing-library/svelte'
+import { userEvent } from '@testing-library/user-event'
+import { describe, expect, test, vi } from 'vitest'
+
+import { IS_SVELTE_5 } from './_env.js'
+import Subject from './fixtures/Binder.svelte'
+
+describe.runIf(IS_SVELTE_5)('binds', () => {
+  test('binding via getter/setter', async () => {
+    const user = userEvent.setup()
+    let value = false
+    const props = {
+      get value() {
+        return value
+      },
+      set value(nextValue) {
+        value = nextValue
+      },
+    }
+
+    render(Subject, props)
+
+    const input = screen.getByRole('checkbox')
+    await user.click(input)
+
+    expect(value).toBe(true)
+  })
+
+  test('binding via getter/setter does not double-trigger effects', async () => {
+    const user = userEvent.setup()
+    const onEffectRun = vi.fn()
+    let value = false
+    const props = {
+      onEffectRun,
+      get value() {
+        return value
+      },
+      set value(nextValue) {
+        value = nextValue
+      },
+    }
+
+    render(Subject, props)
+    expect(onEffectRun).toHaveBeenCalledTimes(1)
+
+    const input = screen.getByRole('checkbox')
+    await user.click(input)
+
+    expect(onEffectRun).toHaveBeenCalledTimes(2)
+  })
+
+  test('binding via $state', async () => {
+    const user = userEvent.setup()
+    const props = $state({ value: false })
+
+    render(Subject, props)
+
+    const input = screen.getByRole('checkbox')
+    await user.click(input)
+
+    expect(props.value).toBe(true)
+  })
+
+  test('binding via $state does not double-trigger effects', async () => {
+    const user = userEvent.setup()
+    const onEffectRun = vi.fn()
+    const props = $state({ value: false, onEffectRun })
+
+    render(Subject, props)
+    expect(onEffectRun).toHaveBeenCalledTimes(1)
+
+    const input = screen.getByRole('checkbox')
+    await user.click(input)
+
+    expect(onEffectRun).toHaveBeenCalledTimes(2)
+  })
+})

tests/fixtures/Binder.svelte

@@ -0,0 +1,13 @@
+<script>
+  let { onEffectRun, value = $bindable(false) } = $props()
+
+  let checked = $state()
+
+  $effect(() => {
+    value = checked
+    onEffectRun?.(checked)
+  })
+</script>
+
+<input type="checkbox" bind:checked />
+<output>checked={value}</output>

vite.config.js

@@ -7,6 +7,7 @@ export default defineConfig({
   test: {
     environment: 'jsdom',
     setupFiles: ['./tests/_vitest-setup.js'],
+    include: ['{examples,tests}/**/*.test.{js,svelte.js}'],
     mockReset: true,
     unstubGlobals: true,
     unstubEnvs: true,