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

Object ownership not added for $state inside $derived #15164

Closed
henrykrinkle01 opened this issue Jan 31, 2025 · 7 comments · Fixed by #15166
Closed

Object ownership not added for $state inside $derived #15164

henrykrinkle01 opened this issue Jan 31, 2025 · 7 comments · Fixed by #15166

Comments

@henrykrinkle01
Copy link
Contributor

Describe the bug

I've been using this pattern from a @PatrickG's comment and I consider it a decent userland-implementation for $state.link:

let count = $state(0);
const linked = $derived.by(()=>{
  const state = $state({ current: count });
  return state;
})
linked.current++;

It worked fine in earlier versions of Svelte 5. However, after the performance patch in PR #14533, object ownership isn't added for $state inside $derived anymore. Quote:

Potentially someone could create a $state while creating $state.raw or inside a $derived.by, but that feels so much of an edge case that it doesn't warrant a perf hit for the common case.

As a consequence, trying to use this pattern across component boundaries, either via the context API or props will produce the ownership_invalid_binding warning, even with proper prop binding. I've seen people asking problems that can be solved perfectly by this pattern, but I'm hesitant to recommend it to them. Sometimes I offer an alternative solution using $effect instead and they're happy to use it because there's no warning. However I feel bad about using $effect because 1. it's not synchronous and 2. updating states inside it is considered a bad practice.

Recently I saw a @paoloricciuti 's comment regarding this pattern and it may be even added to the docs. I'm relieved that it's a valid pattern that is recognized by the team. I feel like it's time the revisit the ownership check for this case again, especially after the recent PR #15153.

Reproduction

https://svelte.dev/playground/d51b66bbb70e413498fe70b23944fcd7?version=5.19.6

Logs

System Info

N/A

Severity

annoyance

dummdidumm added a commit that referenced this issue Jan 31, 2025
- `$derived` can contain `$state` declarations so we cannot ignore them, so this reverts #14533
- instead, we add equality checks to not do this expensive work unnecessarily
- this also adds a render effect similar to the class ownership addition when it detects a getter on a POJO during ownership addition

fixes #15164
dummdidumm added a commit that referenced this issue Jan 31, 2025
- `$derived` can contain `$state` declarations so we cannot ignore them, so this reverts #14533
- instead, we add equality checks to not do this expensive work unnecessarily
- this also adds a render effect similar to the class ownership addition when it detects a getter on a POJO during ownership addition

fixes #15164
@Leonidaz
Copy link

one definite drawback of this linked implementation via a derived is that you can't bind: to the derived as it's not permitted by svelte. So, the only choice is to pass it in as read-only and if writing to it will get a ownership_invalid_mutation warning. It can be ignored though and it's only in dev.

I feel bad about using $effect because 1. it's not synchronous and 2. updating states inside it is considered a bad practice.

for 1, I wonder what's the use case for needing a synchronous update? Deriveds are only rerun when they're read and it would really only happen inside an effect if state changes.

for 2, I would say that it's acceptable to use $effect or $effect.pre to mutate state for this purpose.

@paoloricciuti
Copy link
Member

one definite drawback of this linked implementation via a derived is that you can't bind: to the derived as it's not permitted by svelte.

You can't bind to the derived itself but you should bind to it's property anyway.

for 1, I wonder what's the use case for needing a synchronous update? Deriveds are only rerun when they're read and it would really only happen inside an effect if state changes.

You can read a derived also outside an effect....if you read a linked prop the line after setting the prop with derived you would get the right value, with effect the wrong one.

for 2, I would say that it's acceptable to use $effect or $effect.pre to mutate state for this purpose.

It's way better to do the derived thing

@Leonidaz
Copy link

Leonidaz commented Jan 31, 2025

You can't bind to the derived itself but you should bind to it's property anyway.

right, just assumed the whole thing would be passed in, e.g. if there were more props in the derived vs one by one. I was trying an example where the state was created outside of the $derived (but there is some kind of issue if a derived is read in the component's instance code section which unsafe fixes)

It's way better to do the derived thing

If you need to read it in the instance code then yeah for sure and during ssr. Otherwise, I don't see a problem as long as one understands how $effect works with dependencies and untrack().

Implementing it this ways does seem somewhat cumbersome, I wonder if we had a rune $state.derived that would work just like a derived but be writable at the same time.

e.g.

let count = $state(0);
let state = $state.derived(() => {
  count; // read dependencies, function runs on the first read and on reads if dependencies change

  return count; // or specify whatever conditions make sense,  but able to reference itself (state in this case) vs $derived
});

@paoloricciuti
Copy link
Member

If you need to read it in the instance code then yeah for sure and during ssr. Otherwise, I don't see a problem as long as one understands how $effect works with dependencies and untrack().

It's not only for SSR and during instance (which are also important)

REPL

Changes to derives are immediately live, the same is not true for effects

@Leonidaz
Copy link

that's an interesting use case with events, they do fire before the $effect.pre runs... so you'd have to read the state that gets updated via $effect.pre on the next tick().

@paoloricciuti what do you think of the idea to create $state.derived ?

Implementing it this ways does seem somewhat cumbersome, I wonder if we had a rune $state.derived that would work just like a derived but be writable at the same time.

e.g.

let count = $state(0);
let state = $state.derived(() => {
  count; // read dependencies, function runs on the first read and on reads if dependencies change

  return count; // or specify whatever conditions make sense,  but able to reference itself (state in this case) vs $derived
});

@paoloricciuti
Copy link
Member

@paoloricciuti what do you think of the idea to create $state.derived ?

That's basically $state.link...personally I think it should be desugaree to the derived+state pattern...on the other hand is really not difficult to do in user land and you can even create an utility for it and that would save us from yet another rune. But we are still keeping it in the back of our mind

@Leonidaz
Copy link

Leonidaz commented Feb 1, 2025

right, I just didn't want to name it that since the "api" (and implementation) is different than the one that was proposed originally. Definitely agree that it's basically syntactic sugar but it does make life easier especially for less experienced svelte devs.

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

Successfully merging a pull request may close this issue.

3 participants