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

Spread props cause unnecessary changes #3289

Closed
Rich-Harris opened this issue Jul 25, 2019 · 5 comments · Fixed by #3294
Closed

Spread props cause unnecessary changes #3289

Rich-Harris opened this issue Jul 25, 2019 · 5 comments · Fixed by #3294
Labels

Comments

@Rich-Harris
Copy link
Member

Rich-Harris commented Jul 25, 2019

Describe the bug

Inline expressions are re-evaluated when setting spread props on a component.

To Reproduce

https://svelte.dev/repl/38ab6eb9233a4d8c97e67d576e063109?version=3.6.8

Expected behavior

Waggling the slider should only cause a to show up as having changed. With the spread version, c also appears to have changed.

Severity

Mildly annoying. Could result in incorrect behaviour (as opposed to mere redundant work)

Additional context

This could be fixed like so:

var nested1_changes = (changed.x || changed.a || changed.b) ? get_spread_update(nested1_spread_levels, [
  (changed.x) && ctx.x,
  (changed.a) && { a: ctx.a },
  (changed.b) && { b: ctx.b },
-  { c: [1] },
-  { d: "string" }
+  nested1_spread_levels[3],
+  nested1_spread_levels[4]
]) : {};
nested1.$set(nested1_changes);
@Rich-Harris
Copy link
Member Author

Looking at that code again, I'm wondering if there's a good reason not to do this instead:

if (changed.x || changed.a || changed.b) {
  nested1.$set(get_spread_update(nested1_spread_levels, [
    (changed.x) && ctx.x,
    (changed.a) && { a: ctx.a },
    (changed.b) && { b: ctx.b },
    nested1_spread_levels[3],
    nested1_spread_levels[4]
  ]));
}

@shavyg2
Copy link

shavyg2 commented Jul 26, 2019

Hmm kinda makes sense that it changes, aren't you creating a new array ever time the component is called? I'm kinda thinking that if c should be const shouldn't it be declared in the script at the top as a const?

In react pretty sure c would change on each render and it kinda makes sense

Rich-Harris added a commit that referenced this issue Jul 30, 2019
reuse unchanged spread levels
@Rich-Harris
Copy link
Member Author

No, this is a philosophical difference between React and Svelte. The React ideal is to recreate the universe from scratch on every state change (in practice, it falls short of that for performance reasons), whereas the Svelte ideal is to only ever update things that are known to have changed (in practice, it falls short of that for correctness/simplicity reasons). There's no reason to recreate b in <Foo a={x} b={[1]}/> — no possible value can come of it — so we shouldn't. This is just about aligning the spread prop behaviour with the non-spread-prop behaviour.

@shavyg2
Copy link

shavyg2 commented Aug 1, 2019

I see what your saying at compile time you see there is no reason to change, what if some one spread a value that contains b as a value tho?

@orlov-vo
Copy link

orlov-vo commented Aug 1, 2019

@Rich-Harris Is it related with this bug REPL?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants