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

Runes: get/set syntax / creating reactive #each items sucks #9240

Closed
brunnerh opened this issue Sep 21, 2023 · 8 comments
Closed

Runes: get/set syntax / creating reactive #each items sucks #9240

brunnerh opened this issue Sep 21, 2023 · 8 comments
Labels

Comments

@brunnerh
Copy link
Member

brunnerh commented Sep 21, 2023

Describe the problem

I just wanted to port a simple to-do sort of demo to runes but it was more difficult than expected and the end result is more verbose than a "Svelte" component should be. Of course I might be doing something wrong.

First naive approach was just one state:

<script>
    let todos = $state([
        { text: 'Item 1', done: false },
        { text: 'Item 2', done: false },
    ]);
    const remaining = $derived(todos.filter(x => x.done == false));
</script>

{#each todos as todo}
    <div>
        <input type="checkbox" bind:checked={todo.done} />
        <input bind:value={todo.text} />
    </div>
{/each}
...
Remaining: {remaining.length}

The bindings in the #each block do nothing.

Next step was trying to use a state for each item:

const todo = text => {
    let item = $state({
        text,
        done: false,
    });
    return item;
}
let todos = $state([
    todo('item 1'),
    todo('item 2'),
]);

Which does not work because the item gets evaluated in the compiled output before it is returned.

So finally, every changing property is stateified:

const todo = initialText => {
    let text = $state(initialText);
    let done = $state(false);
    return {
        get text() { return text }, set text(v) { text = v },
        get done() { return done }, set done(v) { done = v },
    };
}

This finally works but is ...not great, even if we could use arrow functions (as already bemoaned by Rich Harris).

Is this just buggy or will it really be necessary to use $state for every single property?
I imagine this being an issue with JSON returned from a server.

Describe the proposed solution

If it is necessary to declare that many properties, that could probably be done by the compiler, e.g. with an additional rune.

const todo = initialText => {
    let text = $state(initialText);
    let done = $state(false);
    return $access({ text, done });
}

Should then be spreadable, too, e.g.

const todo = (id, initialText) => {
    let text = $state(initialText);
    let done = $state(false);
    return { id, ...$access({ text, done }) };
}

$access could possibly be split into something like $readable/$writable to control whether setters are generated.

Alternatives considered

  • Use one $state and bind to that indexed:
    let todos = $state([
        { text: 'Item 1', done: false },
        { text: 'Item 2', done: false },
    ]);
    {#each todos as _, i}
        <div>
            <input type="checkbox" bind:checked={todos[i].done} />
            <input bind:value={todos[i].text} />
        </div>
    {/each}
  • Have one state per item, but wrap it to preserve reactivity (pretty ugly).

Importance

nice to have

@brunnerh
Copy link
Member Author

brunnerh commented Sep 21, 2023

Just realized that #9237 covers similar ground regarding get/set syntax, also making a distinction between what accessors are generated.
Using an object as argument is probably better, though, as it allows generating multiple accessors at the same time and should work with tooling like TypeScript.

Still, I also do question whether #each has to/should be that limited.

@benmccann benmccann added this to the 5.x milestone Sep 21, 2023
@benmccann benmccann removed this from the 5.x milestone Sep 21, 2023
@Gin-Quin
Copy link

It's a crucial point indeed. todo should be reactive if the value of #each is reactive. A simple solution would be for the compiler to transform all todo into todos[i].

@brunnerh
Copy link
Member Author

One of the reasons for runes was to have more fine-grained reactivity, where it would not be necessary to invalidate/update the entire list when one item changes (see intro video / preview docs).

This transform would bring back the previous level of reactivity but also invalidate the entire list.

in runes mode, Svelte no longer invalidates everything when you change something inside an each block. Previously, Svelte tried to statically determine the dependencies of the mutated value in order to invalidate them, causing confusing bugs related to overfiring (invalidating things that weren't actually affected) and underfiring (missing affected variables). It made apps slower by default and harder to reason about, especially in more complex scenarios.

... In runes mode, the rules around triggering updates are simpler: Only state declared by a $state or $props rune causes a rerender. In the broken example, todo is declared by the #each block, and neither the text nor the done property are referencing values of $state runes. One solution would be to turn text and done into $state references, as shown above. The other solution would be to bind to todos[i].text instead of todo.text — this way, Svelte picks up the reference to the todos $state and invalidates it as a whole. Keep in mind that you lose the fine-grained reactivity this way — the whole array is invalidated on every keystroke.

That being the case, this probably has to be approached differently:

  • One has to get used to the new behavior that the #each item variable is no longer magically linked to the source list, so invalidating it will not invalidate the list either. This is kind of more logical, but just not the behavior of previous Svelte versions.
  • Syntax for making the items reactive should be improved as much as possible (see e.g. my suggestion for generating accessors)
  • We may also need a standardized way of making some unreactive data that was constructed elsewhere (e.g. a server response) reactive. I see potential for confusion here and people might be falling back to binding indexed again to get around the new limitations if it is hard/inconvenient to make stuff reactive.

@brunnerh
Copy link
Member Author

brunnerh commented Sep 24, 2023

In Discord Xwth noted that having $state as property initializer would also be nice:

class Product {
  id = $state(crypto.randomUUID());
  name = $state('New Product');
  count = $state(0);
  increment() { this.count++; }
  decrement() { this.count--; }
}

This simply compiling to accessors also seems useful and reasonable.
Right now it is a syntax error because $state needs a declaration (var/let/const).


For the to-do example that would yield something like:

class Todo {
  done = $state(false);
  text = $state();
  constructor(text) { this.text = text }
}
let todos = $state([
    new Todo('item 1'),
    new Todo('item 2'),
]);

If this were to be extended to plain objects, the code really would be rather svelte again:

let todos = $state([
    { text: $state('Item 1'), done: $state(false) },
    { text: $state('Item 2'), done: $state(false) },
]);

(For storing the signals, the class could use private fields or a symbol, the plain object could be compiled to an IIFE that closes over a local variable for the signal and returns an object with accessors.)

@stalkerg
Copy link
Contributor

stalkerg commented Oct 5, 2023

@brunnerh okey, it's ugly but works, what we should do if the source TODO list comes from the server and we serialize it just from JSON?

@brunnerh
Copy link
Member Author

brunnerh commented Oct 8, 2023

@stalkerg You would either have to map it manually to add state or use a utility function to do so. There already has been some discussion on such a function and some version of that will probably be included with Svelte itself.

@stalkerg
Copy link
Contributor

@brunnerh unfortunately, we have no shape of this function yet. It's definitely solvable, but I feel this issue needs more attention.

@brunnerh
Copy link
Member Author

brunnerh commented Dec 5, 2023

Addressed by:

@brunnerh brunnerh closed this as completed Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants