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

Reassignment through object spread inside a loop throwing “Invalid array length” #4574

Closed
sunyatasattva opened this issue Mar 18, 2020 · 3 comments · Fixed by #4945
Closed
Labels

Comments

@sunyatasattva
Copy link

When I try to reassign an object via spread operator inside a loop, it throws an error. However, this works as expected outside loops.

Example

<script>
  let obj = {
    prop: "foo"
  };
</script>

<span class="content">{obj.prop}</span>
<button on:click={ () => obj = { ...obj, prop: "bar" } }>Test</button>

The above works correctly. However, the same logic inside a loop:

<script>
  let obj = {
    prop: "foo"
  };

  let arr = [obj]
</script>

{#each arr as o}
  <span class="content">{o.prop}</span>
  <button on:click={ () => o = { ...o, prop: "bar" } }>Test</button>
{/each}

This throws an Invalid array length error.

Stack trace

Object.update [as p]
https://k01vy.csb.app/App.svelte:233:28
update
https://codesandbox.io/static/js/sandbox.dc9db2542.js:1:179359
flush
https://codesandbox.io/static/js/sandbox.dc9db2542.js:1:179359

Reproduction case

Check this code sandbox: https://codesandbox.io/s/jovial-kapitsa-k01vy

Note that even inside the loop, reassignment through o = o still works, it's only when the spread operator is used that an error is thrown.

@tanhauhau tanhauhau added the each label Mar 18, 2020
@jonniek
Copy link
Contributor

jonniek commented Apr 9, 2020

I think this seems consistent with javascript behavior and is not related to the spread operation. Reassigning the loop variable doesn't update the parent array in javascript.

> arr = [{ prop: "foo" }]
[ { prop: 'foo' } ]
> arr.forEach(o => { o = { prop: "bar" } })
[ { prop: 'foo' } ]

In the node repl it doesn't throw an error but perhaps it's better that svelte does? I'm not sure if there is any usecase where you would want to reaassign the loop variable. If not the compiler could throw a more descriptive error if you try to do that.

Instead of reassigning, I think the correct way is to mutate the object field

> arr.forEach(o => { o.prop = "bar" })
[ { prop: 'bar' } ]

Reassigning the variable within forEach in JS is possible with the third parameter or the array variable

> arr.forEach((_, i, a) => a[i] = { prop: "foobar" })
[ { prop: 'foobar' } ]

> arr.forEach((_, i) => arr[i] = { prop: "foobar2" })
[ { prop: 'foobar2' } ]

In svelte you would need to rely on the second one since the third parameter is not supported in each.

REPL: https://svelte.dev/repl/33fb13b7087b431ba969c397750de399?version=3.20.1

@pushkine
Copy link
Contributor

pushkine commented Apr 9, 2020

https://svelte.dev/repl/88d01dd26f50492b94bd00a25b43d687?version=3.20.1

const click_handler = value => $$invalidate(0, value = whatever);

the compiled code always invalidates the array instead the target

@Conduitry
Copy link
Member

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.

5 participants