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

list is not reactive, but list.length is #12439

Closed
ottomated opened this issue Jul 14, 2024 · 11 comments · Fixed by #12444
Closed

list is not reactive, but list.length is #12439

ottomated opened this issue Jul 14, 2024 · 11 comments · Fixed by #12444

Comments

@ottomated
Copy link
Contributor

Describe the bug

The example in the $effect.pre docs describes referencing a list to trigger an $effect. However, you need to reference list.length instead.

Reproduction

REPL

Logs

No response

System Info

n/a

Severity

blocking an upgrade

@henrikvilhelmberglund
Copy link

You can do $state.snapshot(items);, not sure how stupid/smart this is. Also interested in the official recommended way to do this since the example in the docs doesn't work.

Also a bit annoyed that the docs example isn't a real example that you can paste into the REPL and watch it actually do something.

@ottomated
Copy link
Contributor Author

You can do $state.snapshot(items);, not sure how stupid/smart this is.

Definitely don't want to clone the entire state every time it changes.

@paoloricciuti
Copy link
Member

Describe the bug

The example in the $effect.pre docs describes referencing a list to trigger an $effect. However, you need to reference list.length instead.

Reproduction

REPL

Logs

No response

System Info

n/a

Severity

blocking an upgrade

This is only true if you push to list instead of reassigning, it works if you reassign. That's fine grained at work: if you push to an array only the length will, and probably should, be invalidated.

You can do $state.snapshot(items);, not sure how stupid/smart this is. Also interested in the official recommended way to do this since the example in the docs doesn't work.

It really depends on the use case: snapshot will read every element of the array and add a dependency to it so if you want to react to every change in the array (length, reassign of array, change of an element) then it's the way to go. I'm this case he just want to re-enroll when the length changes so you can read length.

Also a bit annoyed that the docs example isn't a real example that you can paste into the REPL and watch it actually do something.

You are right but that's just because there's no change to messages (and also messages should be state)...if you have time this would be a fenomenale good first issue, otherwise I'll fix that in the next few days.

@PatrickG
Copy link
Member

Because items does not change. The effect would only rerun if you assign a new value to items (items = []).
items.length changes when you .push() to the array.
Example

Also a bit annoyed that the docs example isn't a real example that you can paste into the REPL and watch it actually do something.

Yea, that example should be fixed.

@ottomated
Copy link
Contributor Author

Because items does not change. The effect would only rerun if you assign a new value to items (items = []). items.length changes when you .push() to the array.

Given that it should be encouraged to mutate state like this instead of reassigning it (for performance reasons), I think there should be a warning similar to state_referenced_locally for this:

const items = $state([]);
$effect(() => {
  items;
  // ^ warn: Referencing the root of deeply reactive state will not trigger changes when the state is mutated.
});

Alternatively, a track function (mirroring untrack) that just no-ops, but allows adding a deep reactive dependency on state without the overhead of $state.snapshot:

import { track } from 'svelte';
const huge_form = $state({ /* lots of deep nested data */ });

let has_changed = $state(false);

$effect(() => {
  track(huge_form);
  // or track.deep(huge_form)?
  has_changed = true;
});

@paoloricciuti
Copy link
Member

Because items does not change. The effect would only rerun if you assign a new value to items (items = []). items.length changes when you .push() to the array.

Given that it should be encouraged to mutate state like this instead of reassigning it (for performance reasons), I think there should be a warning similar to state_referenced_locally for this:

const items = $state([]);
$effect(() => {
  items;
  // ^ warn: Referencing the root of deeply reactive state will not trigger changes when the state is mutated.
});

I think directly referencing dependency should be discouraged enough and it's only really needed in rare cases. And given that this is just how fine grained reactivity works I feel like a warning would be kinda wrong (but that's just a gut feeling)

Alternatively, a track function (mirroring untrack) that just no-ops, but allows adding a deep reactive dependency on state without the overhead of $state.snapshot:

import { track } from 'svelte';
const huge_form = $state({ /* lots of deep nested data */ });

let has_changed = $state(false);

$effect(() => {
  track(huge_form);
  // or track.deep(huge_form)?
  has_changed = true;
});

You can't have a track function that doesn't have the overhead of $state.snapshot...to have a dependency on the whole object you have to have a dependency on every property recursively and that's what snapshot does.

@ottomated
Copy link
Contributor Author

ottomated commented Jul 15, 2024

And given that this is just how fine grained reactivity works I feel like a warning would be kinda wrong (but that's just a gut feeling)

I disagree because the items; expression has a clear goal, but because of a complicated implementation detail, does not accomplish that goal

You can't have a track function that doesn't have the overhead of $state.snapshot...to have a dependency on the whole object you have to have a dependency on every property recursively and that's what snapshot does.

This is wrong as of yesterday, see #12413

@PatrickG
Copy link
Member

Alternatively, a track function (mirroring untrack) that just no-ops, but allows adding a deep reactive dependency on state without the overhead of $state.snapshot:

import { track } from 'svelte';
const huge_form = $state({ /* lots of deep nested data */ });

let has_changed = $state(false);

$effect(() => {
  track(huge_form);
  // or track.deep(huge_form)?
  has_changed = true;
});

You can use JSON.stringify(items) which should be faster than $state.snapshot(items).
Otherwise, you could create a little helper that reads all nested signals like this:

function readAll(v) {
  if (typeof v === 'object' && v !== null)
    for (let k in v) readAll(v[k]);
}

$effect(() => {
  readAll(items);
  ...
})

which should be even faster.

Not sure if this needs to be provided by svelte. But I'm not against it either.

@henrikvilhelmberglund
Copy link

Is the new example supposed to autoscroll? I added a button to add messages and it seems like nothing happens - REPL

I think I've used JSON.stringify() for making sure effects update before but it feels a bit strange.

@dummdidumm
Copy link
Member

The example leaves out a style for the div for brevity - maybe we should add it to make that clearer?

@henrikvilhelmberglund
Copy link

The example leaves out a style for the div for brevity - maybe we should add it to make that clearer?

Yes please. If it takes up too much space on the page maybe something like <div style="height:100px;overflow:scroll;" bind:this={div}>

dummdidumm added a commit that referenced this issue Jul 15, 2024
trueadm pushed a commit that referenced this issue Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants