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

[Svelte 5] Runes ($effect, $derived) don't work as expected when values are nested inside an object. #9639

Closed
AgarwalPragy opened this issue Nov 24, 2023 · 16 comments

Comments

@AgarwalPragy
Copy link

Describe the bug

When the state is an object, setting up reactive dependencies within the object is cumbersome / doesn't work as expected

Reproduction

Svelte-4 syntax - small, readable, and works as expected.
REPL

<script>
  let note = {contents: 'Without Runes :)', lowercased: ''};    
  $: note.lowercased = note.contents.toLowerCase();
</script>

<input bind:value={note.contents} />
<pre>{JSON.stringify(note, null, 2)}</pre>

Svelte-5 syntax - doesn't work: ERR_SVELTE_TOO_MANY_UPDATES
REPL

<script>
  let note = $state({contents: 'With Runes :(', lowercased: ''});    
  $effect(() => { note.lowercased = note.contents.toLowerCase(); })
</script>

<input bind:value={note.contents} />
<pre>{JSON.stringify(note, null, 2)}</pre>

Since $derived() can only be used as a variable declaration initializer or a class field, the following doesn't work either

<script>
  let note = $state({contents: 'With Runes :(', lowercased: ''});
  note.lowercased = $derived(note.contents)
</script>

<input bind:value={note.contents} />
<pre>{JSON.stringify(note, null, 2)}</pre>

The solution recommended by @Rich-Harris is to use classes: https://www.youtube.com/watch?v=pTgIx-ucMsY&t=13986s
REPL

<script lang="ts">
  class Note {
    contents: string = $state()
    lowercased: string = $state()

    constructor(contents: string) {
      this.contents = contents;
      this.lowercased = contents.toLowerCase();
    }

    toJson() {
      return {
        contents: this.contents,
        lowercased: this.lowercased
      }
    }
  }
  
  let note = new Note('With Runes :(');
  
  $effect(() => { note.lowercased = note.contents.toLowerCase(); })
</script>

<input bind:value={note.contents} />
<pre>{JSON.stringify(note.toJson(), null, 2)}</pre>

This kinda works, but

  • gives a typescript error Type 'string | undefined' is not assignable to type 'string'
  • is much more code for object creation and serialization

Note that explicitly typing the $state() rune by doing contents: string = $state<string>() doesn't help either - it gives the same typescript error

The only way is to provide an initial value for the state, which seems wrong.

Logs

No response

System Info

N/A

Severity

annoyance

@brunnerh
Copy link
Member

brunnerh commented Nov 24, 2023

Pulling something out of a state object and then updating the same object again in an effect is going to cause issues.

I would just not store the value redundantly in the first place:

let note = $state({
  contents: 'With Runes :(',
  get lowercased() {
    return this.contents.toLowerCase();
  }
});

REPL

The class can also be simplified using $derived in the class:

class Note {
  contents = $state() as string // <- fix for TS error
  lowercased = $derived(this.contents.toLowerCase())
  constructor(contents: string) {
    this.contents = contents;
  }
}

Though the error is a bit of an annoyance in class fields. I deliberated whether I should suggest making the $state type less strict, so it won't add undefined to the type if no argument is given. Would improve this case but make other places less accurate. Decided not to do so, as just moving the annotation to the end and use as is not that big of a deal, you need one anyway if no value is provided.

@Rich-Harris
Copy link
Member

Not in a position to check this right now, but I'm pretty sure you can use a type argument to avoid the as hack:

class Note {
  contents = $state<string>();
  lowercased = $derived(this.contents.toLowerCase());

  constructor(contents: string) {
    this.contents = contents;
  }
}

You can also do this, if you want the properties to be enumerable without adding a toJSON method (not toJson btw — if you have a toJSON method then JSON.stringify will use it without you needing to explicitly call it):

<script lang="ts">
  let contents = $state('With Runes :)');

  let note = $derived({
    contents,
    lowercased: contents.toLowerCase()
  });
</script>

<input bind:value={contents} />
<pre>{JSON.stringify(note, null, 2)}</pre>

The ERR_SVELTE_TOO_MANY_UPDATES error is a clue to why the $: note.lowercase = ... pattern is dangerous and not recommended — the code should re-run when note is mutated (since the note.contents.toLowerCase() part of the equation is obviously dependent on the value of note, which just changed), but since that results in an infinite loop we prevent that from happening. In this case it happens to result in the desired outcome, but in many other cases it causes stuff to break.

By using $derived, you're expressing the flow of data in a much more sensible way, and Svelte is able to do the correct thing reliably as a result.

@brunnerh
Copy link
Member

brunnerh commented Nov 25, 2023

Just using the regular generic type arg does not work, as stated the type definition adds undefined after the fact if the argument is missing:

declare function $state<T>(initial: T): T;
declare function $state<T>(): T | undefined;

@Rich-Harris
Copy link
Member

Hmm. I think we should probably change that to this:

declare function $state<T>(initial?: T): T;

It might not be technically correct, but T | undefined is rather unhelpful

@brunnerh
Copy link
Member

It's only really an issue with class fields. In other situations you usually have the initial value at hand already, for classes it first has to be passed in via the constructor. It is also still left to be seen how much classes will be used in the end.

That is why I had not suggested that change already.

@brunnerh
Copy link
Member

brunnerh commented Nov 25, 2023

Wouldn't mind the change much either, though.
One can still manually add | undefined at the usage site or add the more strict type declaration locally, if so desired.


Just remembered that definite assignment assertions would work here, too:

class Note {
  contents = $state<string>()!;
  ...
}

@dummdidumm
Copy link
Member

If this is solveable with x = $state() as string then I'm against changing the signature.

@Rich-Harris
Copy link
Member

In the past we've been opposed to using as like this. It feels weird to explicitly add a type argument but then have it be ignored. As far as correctness goes, as and ! are no more correct than setting the type from the type argument, and arguably less so.

Stuff like this is a real nuisance:

image

What if we simply checked that uninitialized state fields are set during the constructor? It would get annoying if the constructor contained logic or method calls, though.

@dummdidumm
Copy link
Member

dummdidumm commented Nov 26, 2023

We've been against as in different contexts. In this context you're not initializing the state in the declaration at the top and as such none of the gotchas/drawbacks of as applies. Changing the type declaration to be less correct means we allow for bugs to slip through in many more situations just so typing this specific case gets a tiny bit easier (it's only two whitespace characters you type more in comparison to the generic variant).

Another solution - one we may need to add anyway because of how typescript may transpile this - is to allow people to initialize the variable with $state in the constructor, too, if the variable is set to the same type of rune in all branches of the constructor

@AgarwalPragy
Copy link
Author

class Note {
  contents = $state<string>()!;
  ...
}

That doesn't seem to work, at least in the REPL

@brunnerh
Copy link
Member

Maybe a REPL issue, it worked locally and it appears to work in SvelteLab (try this template: https://www.sveltelab.dev/?t=five).

@mjadobson
Copy link

Just to add to this issue: you still get ERR_SVELTE_TOO_MANY_UPDATES even if the $effect isn't self-referential:

<script>
  let text = $state('initial');
  let state = $state({});    
  $effect(() => { state.contents = text; })
</script>

text: <input bind:value={text} />
state.contents: <input bind:value={state.contents} />

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE22P0QqDMAxFfyWUgQpjvlcd7DvmHpxGCNRYbJSN0n9fuz2IsLzd3HNviFcjGXRK373ibkKl1c1adVbytkm4DY1g1G5elz5tatcvZOXacisGBQRfAg2cnHSCeUZMQp3JiqplgAR8jZ3woaggTrJPOI7YS54X0FzB_9BLP7Mgi4uZVF5BKFquy_0up7WGmtiuAk_iQW-dWbHxyQhQRuhY9Rc-IikW_5zmgUbCQWlZVgyP8AH20R9xIgEAAA==

dummdidumm added a commit that referenced this issue Nov 28, 2023
dummdidumm added a commit that referenced this issue Nov 28, 2023
prevents infinite loops when mutation happens inside an effect
came up in #9639
Rich-Harris added a commit that referenced this issue Nov 28, 2023
* fix: handle ts expressions when dealing with runes

related to #9639

* docs, more tests

* simplify

---------

Co-authored-by: Rich Harris <[email protected]>
@AgarwalPragy
Copy link
Author

I guess #9739 resolves this issue? No need of classes now!

@niemyjski
Copy link

tracking down a bug with why something wasn't round tripped to storage due to a rune and the class missing toJson was pretty frustrating.

niemyjski added a commit to exceptionless/Exceptionless that referenced this issue Jul 24, 2024
@niemyjski
Copy link

niemyjski commented Jul 24, 2024

@Rich-Harris really feels like the compiler could emit toJSON for any runes/props inside of a class. That would be a really good dx experience and work just like 4.x.

@Rich-Harris
Copy link
Member

Please don't comment on closed issues, especially with something that isn't directly related to the issue at hand. It's unlikely that the comment will be acted upon. There's a new issue button right there

niemyjski added a commit to exceptionless/Exceptionless that referenced this issue Jul 30, 2024
* Updated to svelte5 dep

* added link resources

* updated client hooks

* Commented out legos

* removed unused code

* added temp tanstack svelte5 adaptor from pr

* WIP rework data tables for svelte5

* Updated svelte

* Temp impl for svelte persisted store

https://twitter.com/puruvjdev/status/1787037268143689894/photo/1
joshnuss/svelte-persisted-store#251

* WIP Runes

* More svelte 5 conversion

* Updated package json

* WIP Svelte 5 work

* Upgraded typography components to runes and use new snippets and props

* More runes work

* More Svelte 5 work

* Updated facet filters to runes

* Upgraded to @exceptionless/fetchclient

* Updated deps

* Upgraded to latest fetch client

* Updated shad

* Upgrade to eslint 9

https://github.com/sveltejs/kit/pull/12268/files

* fixed invalid ref

* Converted slots

* Updated deps

* Added some snippets

* WIP Dashboard

* Updated tailwind

* Fixed copy

* Fixed slots

* Updated deps

* Updated data tables and summaries

* More conversion

* Updated deps

* Converted all components to runes

* Some dispatch changes

* WIP: Dispatch Changes

* Updated deps

* more dispatch work

* Fixed linting errors

* Updated svelte

* Updated shad

* Updated typescript

* Updated deps

* Fixed fetch client

* Fixed svelte errors.

* added conditional icon check

* Update deps

* WIP: Use svelte-query-runes

TanStack/query#6981

* WIP - Reworked snippets and some other fixes.

* Updated deps

* WIP - Svelte 5 work

* Updated deps

* Updated to latest tanstack table

* WIP: Filter runes work.

* Fixed more errors.

* Updated deps

* Use svelte query pkg

* Updated svelte and vitest

* Fixed svelte query issues, fixed linting issues, fixed eventing.

* Fixed web socket events

* Updated deps

* Fixed some filtering issues

* Fixed persisted store

* Updated packages

* Fixed filter reactivity

* Updated deps

* Updated deps

* fixed svelte query issues

* Use runed media query

* Update deps

* Use runed media query

* Fixed svelte issue with state_unsafe_mutation

sveltejs/svelte#12571

* Ensure all runes are serialized with toJSON

sveltejs/svelte#9639

* Added document visibility store and fixed more upgrades

* Updated deps

* Updated queries

* WIP: Demoting tabs

* Updated deps

* Updates to tab promotion

* Fixed bind warnings

* Updated deps

* Use new svelte query apis

* Updated deps

* Fixed stack promotion

* Fixed grid issues

* Fixed fetch client issues with the login forms

* Added custom auth serializer

* Force package install due to svelte 5 peer deps

* rebuilt lock file

* Update Document Visibility helper to match new runed pattern
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

6 participants