Skip to content

Get rid of each_item_invalid_assignment in svelte 5 #13592

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

Closed
JonaAnders opened this issue Oct 13, 2024 · 9 comments
Closed

Get rid of each_item_invalid_assignment in svelte 5 #13592

JonaAnders opened this issue Oct 13, 2024 · 9 comments

Comments

@JonaAnders
Copy link

JonaAnders commented Oct 13, 2024

Describe the problem

When converting a component from svelte 4 to svelte 5 (runes mode), I noticed the each_item_invalid_assignment error. The error message says "Cannot reassign or bind to each block argument in runes mode. Use the array and index variables instead (e.g. array[i] = value instead of entry = value)". That means it is no longer possible to write

<script lang="ts">
    interface Props {
        values: number[];
    }
    let { values }: Props = $props();
</script>

{#each values as value}
    <button onclick={() => value++}>Click me!</button>
{/each}

Instead, I now have to write

<script lang="ts">
    interface Props {
        values: number[];
    }
    let { values }: Props = $props();
</script>

{#each values as value, i}
    <button onclick={() => values[i]++}>Click me!</button>
{/each}

Describe the proposed solution

For me, it seems like it would be fairly straight forward to just let the compiler replace value with values[i]. That way, the code would be a little cleaner to write and would stay compatible with the svelte 4 way, while at the same time satisfy the underlying constraint that makes this necessary.

Importance

nice to have

@7nik
Copy link
Contributor

7nik commented Oct 13, 2024

The passed collection can be not-an-array, so you cannot just do values[i].

@JonaAnders
Copy link
Author

The passed collection can be not-an-array, so you cannot just do values[i].

That seems like a good reason for it to be pretty hard (if not impossible) to implement. Thanks for taking the time!

@JonaAnders JonaAnders closed this as not planned Won't fix, can't repro, duplicate, stale Oct 13, 2024
@adiguba
Copy link
Contributor

adiguba commented Oct 14, 2024

Hello,

I think that is not a sufficient reason :

Personally I think the each_item_invalid_assignment compile-error should be removed, and replaced by a runtime-error when the collection is not an array.
Something like "Binding to a non-array collection in #each is not allowed"

@Conduitry
Copy link
Member

It works in non-runes mode because the invalidation is not fine-grained. The whole array gets invalidated.

@adiguba
Copy link
Contributor

adiguba commented Oct 14, 2024

I just find this restriction strange : if bind:value={arr[i]} work, the compiler should be able to generate the correct code for bind:value={item}...

And actually I just tested it simply by commenting out the each_item_invalid_assignment error... and it's just work.
=> Like in non-rune mode it generate something like this for bind:value={item} :

$.bind_value(input, () => arr[$$index], ($$value) => (arr[$$index] = $$value));

I must be missing something... or it is an arbitrary constraint.

@7nik
Copy link
Contributor

7nik commented Oct 15, 2024

Of course, it will "work" - you can add props to any object unless it is frozen or custom setters reject writing. However, the iterator is still unaware of these new props and will return the old values.

arr[i] = ... can be used only on arrays and array-like objects but not iterable objects. You should use types to ensure the collection is array(-like) or tell that the component accepts only array.

@adiguba
Copy link
Contributor

adiguba commented Oct 15, 2024

In case of bind/assignment, instead of throwing each_item_invalid_assignment, the compiler could just pass a flag to $.each() in order to require an array.

=> So if I use that with a non-array, I will get a runtime-error...

@7nik
Copy link
Contributor

7nik commented Oct 15, 2024

I think I found the reason by Rich Harris:

I think it should be disallowed, because otherwise things like this break down:

-{#each numbers as n}
+{#each numbers.filter((n) => n % 2 === 0) as n}
  <button onclick={() => n += 1}>{n}</button>
{/each}

It's better to enforce consistency (e.g. 'you have to do numbers[i] += 1') than to have something that is superficially more convenient, but which is pretty confusing on the face of it (it's basically like reassigning a function parameter, and yet it somehow works?) and which requires you to understand non-obvious properties of the system before you can wield it successfully.

Example

So, in cases like {#each getItems() as item} or {#each foo.getData().items as item}, the compiler cannot know whether it makes sense to mutate the array.

Edit: It was initially forbidden because there was no proxy state; thus, it was impossible. Later, after introducing the proxy state, they decided to keep it for the reason above.

@adiguba
Copy link
Contributor

adiguba commented Oct 15, 2024

Yes this reason is more understandable.
Thanks

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

No branches or pull requests

4 participants