sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
76.98k stars 4k forks source link

Svelte 5: When spread object passed to components changes all the output of $props is updated instead of only the necessary stuff #11286

Closed HighFunctioningSociopathSH closed 2 weeks ago

HighFunctioningSociopathSH commented 1 month ago

Describe the bug

When {...restProps} passed to a component changes, all the other props also elicit a reaction even though nothing is changed. This false signal causes components to almost completely render again which is a waste of resources and can also cause bugs.

Reproduction

lets say we have the following component // Test.svelte

<svelte:options runes />

<script lang="ts">
  let { about, ...restProps }: { about?: string; someObject?: Record<string, any> } = $props();

  $effect(() => {
    console.log(about); // This runs again when restProps is changed.
  });
</script>

And in our +page we render it like below

<script lang="ts">
  import Test from "$components/Test/Test.svelte";
  import { onMount } from "svelte";

  onMount(() => {
    setTimeout(() => {
      restProps = {};
    }, 3000);
  });

  let restProps = $state({});
</script>

<Test about="something" {...restProps}></Test>

Now changing restProps will cause the $effect that is looking at the primitive value of about to run again and this happens for all props.

If our restProps is the output of a $derived then it will be a new object every time that derived runs again which means any reactive code inside Test runs again.

One bug that this can cause is for example if you have a functionality that is based on changes to a prop then that functionality will run again even though the user didn't change that prop at all. for instance, In the following example from the documentation that uses the writable derived method which is supposed to update the value of facade whenever the incoming prop value changes but also allow for changes to facade from within the component, The false signal causes the getter to go off even though the value prop was not changed by the user which results in facade to have the wrong value.

// Test.svelte
<script lang="ts">
  let { value, ...restProps }: { value: string; someObject?: Record<string, any> } = $props();

  let facade = {
    get value() {
      return value.toUpperCase();
    },
    set value(val) {
      value = val.toLowerCase();
    }
  };
</script>

<div>
  Test input
  <input bind:value={facade.value} />
</div>

// +page.svelte

<script lang="ts">
  import Test from "$components/Test/Test.svelte";
  import { onMount } from "svelte";

  onMount(() => {
    setTimeout(() => {
      restProps = {};
    }, 5000);
  });

  let restProps = $state({});

  let value = $state("hello");
</script>

<Test {value} {...restProps}></Test>

<div>
  +page input
  <input bind:value />
</div>

Here try to change the value of Test input then wait 5 seconds to witness that even without typing in the +page input the value of Test input changes to +page input's value because restProps was changed

REPL

Logs

No response

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (16) x64 12th Gen Intel(R) Core(TM) i7-12650H
    Memory: 5.77 GB / 15.63 GB
  Binaries:
    Node: 18.14.2 - C:\Program Files\nodejs\node.EXE
    npm: 9.7.1 - C:\Program Files\nodejs\npm.CMD
    bun: 1.1.3 - ~\.bun\bin\bun.EXE
  Browsers:
    Edge: Chromium (123.0.2420.97)
    Internet Explorer: 11.0.22621.1
  npmPackages:
    svelte: ^5.0.0-next.110 => 5.0.0-next.110

Severity

annoyance

paoloricciuti commented 1 month ago

Interestingly the order of the props matters: repl

HighFunctioningSociopathSH commented 4 weeks ago

@trueadm Here's another example of a problem I think has to do with the same thing and it's weird. Lets say we have the following component. Test.svelte

<script lang="ts">
  import { onMount } from "svelte";

  let { open = $bindable(false), about, somethingElse }: { open?: boolean; about?: string; somethingElse?: number } = $props();

  // $inspect(open);
  $effect(() => {
    console.log(open);
  });

  onMount(() => {
    setTimeout(() => {
      open = true;
    }, 3000);
  });
</script>

And this is our +page where I'm simulating the spreading of 2 different objects based on a condition.

<script lang="ts">
  import Test from "$components/Test/Test.svelte";
  import { onMount } from "svelte";

  let boolVar = $state(false);
  onMount(() => {
    setTimeout(() => {
      boolVar = true;
    }, 5000);
  });
</script>

<Test {...boolVar ? {} : { about: "hello", somethingElse: 2 }}></Test>

Now after 3 seconds, we are simulating a change from inside Test that sets open to true, then wait for the setTimeout inside page to change the boolVar variable. You will notice that the variable open which was not even in the spreaded object, retakes its default value, meaning it changes back to false. Now if you change the $effect with an inspect then something else happens. Even though the reactivity is triggered and $inspect runs again, this time the value of open remains true.

paoloricciuti commented 4 weeks ago

Just to clarify the behaviour: the reason effects rerun when you reassign a spread is because they need to check if the new shape of the object is overriding a prop. In my PR I limited this by listening to a derived of the keys of the object so that it's only rerunning when the keys change.

However it would still need to rerun in both cases you proposed.

HighFunctioningSociopathSH commented 4 weeks ago

but aren't props like open or about primitives? shouldn't it run when the value changes? there is no mention of the parent object inside the $effect. still, it shouldn't cause open to take its default value again.

paoloricciuti commented 4 weeks ago

but aren't props like open or about primitives? shouldn't it run when the value changes? there is no mention of the parent object inside the $effect.

What I mean is this:

<Child open={true} {...rest} />
<button onclick={()=> rest = { open: false }}>change</button>

So if inside the effect you access open you have an implicit dependency on the object because it the object changes open could change

Rich-Harris commented 2 weeks ago

After discussion, we decided against merging #11290 — this is one of those cases where the cure (creating extra derived signals indiscriminately) is worse than the disease (effects occasionally overfiring). Effects should generally be idempotent, meaning it doesn't matter if they overfire, and in the cases where it does matter you can create a derived locally to work around the problem.

The case where a prop is reset to its default value is admittedly trickier. But #11290 doesn't actually solve it, it only mitigates it, because you could easily have a situation where the keys of the spread prop change.

If we want to fix that, I think we would need to have a new rule along the lines of 'if an unbound prop was changed locally, it won't be reset to the fallback value if the parent changes it to undefined'.