-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat: allow $derived($state()) for deep reactive deriveds
#17308
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: e846ebb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
YES. let local_arr = $derived.by(()=>{
let _ = $state($state.snapshot(arr));
return _;
});Which translates to let local_arr = $derived($state($state.snapshot(arr)));It's up to debate whether this should be the default behavior |
|
Alternatively, we could expose a This would also have the advantage of working in a universal |
I have been doing this for months in userland by exporting functions that wrap |
|
This would be most helpful. After the recent change to warn In terms of naming/conventions, I've also seen the suggestion in some other issues for <script>
const { client } = $props()
let cats = $state.from(() => {
client.cats.map((cat) => ({
...cat,
_id: Math.random(),
})),
})
function addCat() {
cats.push({
_id: Math.random(),
})
}
</script>
{#each cats as cat (cat._id)}
<input type="text" value="{cat.name}" /><br />
{/each}
<button onclick={addCat} title="Add Cat">Add Cat</button>In the above, if the client prop changed, any $state.from would get recalculated |
|
Pretty sure any version where $state is the leading text is wrong. $state denotes a source. Which this is not, this derives it's source. |
|
My preference would be But I'm also fine with |
|
One question with this approach would also be reassignments. let thing = $derived($state(array));
thing = otherArray;Is So having a thing = $state(otherArray); // not allowed
thing = proxify(otherArray); // OK |
|
The function proxify(value) {
const proxified = $state(value);
return proxified;
}
let foo = $derived(proxify(...));src: #16189 (comment) |
If you reassign to the derived, it will break reactivity, yes. If you reassign to the prop/state it's based on it will work correctly. https://svelte.dev/playground/1a0230ade38a4ddd8ebc64ecfa8c5d95?version=5.45.6 This foot gun would exist regardless of whether you use |
|
To avoid reassignment: function box<T>(value: T): { current: T } {
const boxed = $state({
current: $state.snapshot(value)
});
return boxed;
}
const thing = $derived(box(array));
thing.current = otherArray; |
The difference is that in one case you can avoid the reactivity loss using the same syntax ( And as noted, if the wrapping happens inside a dedicated rune, the problem would not exist at all. |
|
I think it's possible to make |
That still results in reactivity loss. https://svelte.dev/playground/663f9bf9fe4e420c8b398b6b83256d16?version=5.45.6
That does more intuitively help you avoid the foot gun. Though the foot gun still exists if you reassign to thing. However, it's at least mitigated because you're far more likely to reassign to If not making a new rune, this is the method I'd probably go with. Otherwise, as stated, you'd probably need another warning. |
|
Sorry if this question makes you smile (I'm the least appropriate person to answer these technical questions): why can't we have a deep reactive |
|
My assumption is that |
|
For minimal magic-guts-exposing (exposing the behavior without lifting the nuts-and-bolts of how reactivity proxies work right to the user's face), maybe some extensions on derived would serve well? const thing = $derived.proxy(array);
const thing = $derived.proxyOf(() => {
// if possible
return array
});That would satisfy:
Edit: not as a replacement for what this does, but an extension. |
This is the most compelling argument I've seen for a new rune. |
|
I'm not sure Some options being tossed around + some new ones:
My personal preference is for options 1 or 3, but given how often I need this in our codebase, I'd be happy with any of them :-) Another alternative would be to allow a way to make $derived deeply reactive (substitute
|
|
A significant argument against a rune in my view is the inability to use it in a SvelteKit The name |
This PR/discussion is about making deeply reactive As for the name, Svelte doesn't really hide that it uses proxies as an implementation detail, but perhaps Personally I really like |
|
When coming up with a new API, all cases needs to be considered, not just the current one we opened an issue for. This is what Rich does and why we enjoy it. The buttery smooth vibes. My attempt at naming: |
Why do you even need to use this feature in the load function? I think this is a client side UI thing only. I can understand when you want to My arguments against supporting the load function:
A new rune is better than any wrapper solution because:
As for naming, I vote for either |
+1 for $derived.deep. Pretty much explains itself and follows the same convention as writing $derived.by |
I like this. If this doesn't get implemented, it could be an idea to run by the Runed library. |
Kinda already existing though. You can check out various |
|
@paoloricciuti As mentioned in some of the above comments, reactivity is lost when reassigned. I think any PR to fix this should make sure reactivity isn't lost when it is reassigned. So it might pay to add/modify a test case to your PR: <script>
let { arr } = $props();
let local_arr = $derived($state(arr));
function add() {
local_arr.push(local_arr.length + 1);
}
function pop() {
local_arr = local_arr.slice(0, -1)
}
</script>
<button onclick={add}>Add</button>
<button onclick={pop}>Pop</button>
{#each local_arr as item}
<p>{item}</p>
{/each}If you click "Add", it works, and the first time you click "Pop" it will also work, but then using "Add" after that is now broken, because the "Pop" lost the $state aspect and became a shallow derived. It's a simple example (obviously the above could just use Would be great to have some Svelte core-team members weigh in on this. I know they are all very busy on the async functions for Svelte 6. But it would be great to know if we missing something obvious that would allow us to use props in $state or an existing way to make $derived deeply reactive. If not, is there any interest in making something like |
|
I mean if it is working with state just ignore the warning. Personally I would check if it is really working or if there's some underlying bug which this warning just highlighted. If the latter is true I would try to figure out why do you need this behavior...it doesn't seem super common to me and if you have some realistic example I could maybe help you in that case. I feel like this is not a desirable behaviour most of the times (which is partially the reason deriveds are not deep in the first place) |
|
@paoloricciuti It is working, but we would prefer not to litter our codebase with ignore comments, and if we disable it globally, we do risk missing legit issues. So we'd prefer to fix the issues. Our use case is that on page load, it fetches the current state of the records. It then passes various parts of that initial state to various child components. Those child components then create a $state from the initial props, add, change, or remove records as needed, and then use the initial props and the modified state to calculate differences to reduce network traffic. https://svelte.dev/playground/591d1d6653a54d77ad9697413bae29a3?version=5.45.8 The playground works well, but you'll see the svelte warning in AssignmentRules.svelte. If you change that to $derived (as the warning suggests), then other things start breaking (such as the bind:value). If you change it to be the This pattern is something we have used 100s of times in our codebase. We need a way to create a $state object (i.e. deeply reactive) from props. But there doesn't seem to be a good way to do this right now that doesn't break after reassignment. |
|
You can do this with somewhat minimal code changes in that does involve the rest of the code those https://svelte.dev/playground/a7d32039c63144a984eeae8fcbb0d079?version=5.45.8 However this code feels brittle, the warn is 100% right here: the moment you change the parent component to so that the props can change it will break...and tbf I'm not even sure how your diff is working...if you pass a proxied state to $state it just return the same reference so when you push you are pushing to the same array. |
|
@paoloricciuti I simplified the example, our production code is a lot more complex than this. Making the changes you suggested would require a lot of refactoring on our end. We already had a heck of a time upgrading from Svelte 4 (we used to use A way to create a derived value that returns a deeply reactive object seems to be something a number of people need. |
This needs discussion, but Dominic and I had this idea. Deriveds are not deeply reactive, but even nowadays you can kinda achieve deep reactivity for deriveds by doing
This is fine, but it's a bit boilerplatey. This PR is to allow a shorthand of this by allowing
$stateto be used in theinitof a derived.This basically gets compiled to
So it works exactly the same.
Point of discussions:
$stateand not$state.rawyou are actually mutating the parentHowever, this is also true for the original trick that we currently "kinda" recommend.
I look at the server output, and it doesn't need any changes, since
$stateis just removed anyway.WDYT should we do this?
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint