-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: decide on behaviour of $state.frozen
#12434
Comments
The
|
I would suggest keep this api as close as Svelte 3/4's behaviour (no proxy only source), and not treat dev and prod differently (as @brunnerh suggested leave freeze to user land). It's a perf enhance / advanced api, user who pick this over In my current codebase, I use a lot of object as a complex immutable value type, for instance |
My thoughts:
|
I don't think we can do that. If you remember that's what original |
Personally, I'm in favour of keeping |
That feels like the worst of all options honestly — extra surface area, and it would be very hard to articulate when to use which. To the best of my understand/recollection (which may not be fully accurate) the reason we added the symbol was to deal with this scenario: let object = $state.frozen({ x: 1 });
let array = $state([object]); // oops, `array[0]` is now a proxy, we didn't want that Svelte doesn't proxify objects with the symbol, meaning Currently though, we don't have a way to express 'this thing is reactive — we can do let object = $state.raw({ x: 1 }); // reassigning (but not mutating) `object` will cause updates
let array = $state.shallow([object]); // we can push/pop/etc, but the members of the array are untouched
// or...
let array = $state.shallow([{ x: 1 }]);
array[0].x += 1; // triggers no updates (should probably yield a warning, on a best-effort basis) |
This is maybe offtopic and I'm not requesting the rationale, just offering a thought expressed by others in various forms - is deep reactivity the best default? With limited experience I think I'd prefer It would feel like less of a surprise/risk if There's also the idea switching to the less elegant |
If it's really "only" about arrays, there are other solutions I think would be better:
|
Wouldn't that just be this? let position = { x: 1, y: 1 };
let transform = $state.shallow({ position });
let obj = $state.shallow({ transform }); |
Gah, GitHub didn't show me more recent replies. Editing:
There's definitely room for debate on this topic, but so far when we've had that discussion we've landed on deep reactivity being the right default — the idea being that by using the I suspect that for many people, the question arises precisely because we don't have an opt-out mechanism at present. It's clearly a gap in the API (and one that the aforementioned systems cover with
It's definitely solvable, yeah. But I think 'I want reactivity two levels deep' is a real edge case anyway |
Thinking about it more, do you need a let obj = $state({
transform: {
position: $state.raw({
x: 0,
})
},
material: {
colour: {
r: 1,
},
}
});
// ...elsewhere
obj.transform.position.x = 2; // Does NOT trigger reactivity, as reactivity "stops" at `position`
obj.material.colour.r = 0.5; // Does trigger reactivity If so that seems like a pretty clear way to carve up which bits of an object should be reactive, and which bits shouldn't. Just wrap things you don't want to be reactive in |
I'm more leaning towards |
I don't think so. For one thing, you'd need to add a symbol to the But the larger issue is that you would need to use it on every assignment... obj.transform = {
position: $state.raw({
x: 1
})
}; ...otherwise class Raw {
constructor(properties) {
Object.assign(this, properties);
}
}
let obj = $state({
transform: {
position: new Raw({
x: 0,
})
},
material: {
colour: {
r: 1,
},
}
}); It's also much more annoying to do this... const swatch = $state([
$state.raw(createColour('red')),
$state.raw(createColour('orange')),
$state.raw(createColour('yellow')),
$state.raw(createColour('green')),
$state.raw(createColour('blue')),
$state.raw(createColour('indigo')),
$state.raw(createColour('violet'))
]); ...than this: const swatch = $state.shallow([
createColour('red'),
createColour('orange'),
createColour('yellow'),
createColour('green'),
createColour('blue'),
createColour('indigo'),
createColour('violet')
]);
I thought we were leaning towards getting rid of the dev/prod discrepancy? The main reason to use this thing — whatever form it takes — is that you have a large blob of data that you don't plan to mutate, and so you're opting out of deep anything.
I don't think that's right. You're not declaring something with |
Almost every immutable library I know makes use of
Up until this point, every use-case has been for it being mutable by "third party" things. There's half a dozen issues related to this, and so I feel like people will just use it for that intention. I've got strong mixed feelings on |
Really? I feel like the primary use case is more like this: let data = $state.raw();
poll('/some-endpoint', 5000, (v) => {
if (data === undefined || v.timestamp > data.timestamp) {
data = v; // big blob of data that will never be mutated
}
}); If I was still doing my previous job at the NYT, building visualisations of big hairy datasets, then that's exactly what I'd want to do (and I'd be pissed if Svelte took it upon itself to call |
I just see it being used to wrongly solve #12364 |
I see it like this:
So yes, it can be used to do things which are not forkable. But the people writing code in such a way probably are not the people who care about a world in which they can do forking, and/or cannot make use of it anyway, because they want to / have to integrate with third party libraries which have different requirements (#12364 actually is a good example of this, as it's not practical to require people to not use certain patterns if that's the only choice they have to integrate with something). |
You just described classes though. Which are a superior primitive in Svelte 5 for this purpose. If people are using object literals and arrays in state, they will make mistakes, regardless of interference from the framework. I know this so well first-hand working on React. I stand by what I said before, strongly too. |
But it's not practical to force classes upon people, again they might even be able to use them because the integration requires them to pass in POJOs, or gives POJOs back out which you then would need to class-ify somehow. |
I’ve not seen any convincing cases of that outside of examples where they can easily be adapted. I think it’s more of an educational thing. |
I don't think you can easily adapt (if at all) all use cases. The third party library might have certain requirements about the state passed to it (like it not being frozen or proxified), or (more common) the third party integration will give you a POJO and it would be super annoying/cumbersome to convert those to classes just so that they are not proxified. Take Rich's example: poll('/some-endpoint', 5000, (v) => {
if (data === undefined || v.timestamp > data.timestamp) {
data = v; // big blob of data that will never be mutated
}
}); If I had to convert the whole thing to classes just so that they are left alone would be imperformant and very annoying to do - maybe even impossible if you don't know the shape up front. Also, if your solution is to use classes to mutate things, then that still means things can be mutated and therefore not reversible from a fork anyway, so nothing is won. |
That compromise isn’t what Rich is saying above though. He’s not mutating the object. |
Yes, but he also doesn't want it to be frozen or have a symbol attached or anything - it should be left alone. And and others wouldn't want to mutate the object either, unless they have to because their third party integration does require it. |
Are you saying that the 'correct' solution is to do this? poll('/some-endpoint', 5000, (v) => {
if (data === undefined || v.timestamp > data.timestamp) {
- data = v;
+ data = new Raw(v);
}
}); Because that is absurd! Classes are absolutely not the right tool for this job, and nor is freezing. Other systems have an equivalent API, there's no reason we shouldn't. |
No, I'm not.
So from my point of view, we should aim to have the guarantees of immutability as that enables some powerful future provisioning around features. So that means we either have mutable pseudo immutability with signals ( |
What then?
The use of
Forcing |
Again, that just doesn't fly with me. Not unless we add a DEV time warning that it was incorrectly used. The whole "it's on them" has never scaled in frontend frameworks, it just leads to people avoiding future upgrades because the level of effort required to change over likely isn't worth it to them. I'm being stubborn with this because it's a topic that is close to my heart and I have a great deal of experience trying to tackle migrations that have suffered from this. For example, if you suggested that in DEV we deeply put each object literal/array into a WeakMap, and then wherever we have an object mutation the compiler adds in a check to see if that object was in the WeakMap – then you'd likely capture these cases without the need to use proxies. |
The sorts of cases I'm describing are graphics like this election results page, which is powered by (inter alia) this 6.5MB blob of JSON. Assuming that we don't want to use There's a much bigger problem though: <script>
let element = $state.raw();
function update_document_title() {
document.title = 'updated'; // oops! element.ownerDocument is frozen
}
</script>
<div bind:this={element}>...</div> If we have to say to people ' Maybe there are some tricks we can use to avoid those cases — ignoring accessors, or only freezing POJOs — but it feels like a game of whac-a-mole that will end up with a convoluted set of rules that no-one really understands. There's a difference between guiding users towards successful outcomes and treating them like children, and this feels like the latter to be honest. |
We gotta meet people where they are - we can't force everything through the Svelte-way of doing things. That would be very much the opposite of what Svelte 3/4 did. Also in other places we say "if you want to mutate things, use a class", so we're already saying that there ways around this. And this is the reality: There will always be ways around this. JS as a language isn't very good at immutability, and so we can't design future work under the premise that everything is. What we can do is design new APIs that would under certain circumstances not work quite correct because you can't revert things because you didn't play by the rules, but that's on you. But what we absolutely cannot do is design APIs that people are forced to use that also force them to use immutability. |
As a side note, you most definitely can. It's clear that you and Rich don't want to do that though, so I'm happy to leave that conversation. However, I still feel we need to do more here than just invent |
@Rich-Harris Can I ask what is the reasoning that something like this warrants a warning (also there are lots of other missuses that produce warnings and saved me bunch of times, I really love svlete cuz of these DX things), but this doesn't (which is a LOT more prone to bugs IMHO) and should be resorted to docs only? I hope that I do understand your point, if I see a variable named like |
Because per #12434 (comment) it doesn't seem like it is technically possible |
I agree with the notion of providing more flexibility with
This would be the cherry on top. Svelte 5s ergonomics already excel in most use cases but implementing more complicated shenanigans is hard/impossible. This proposal would open up paths for more controlled optimizations in user land, especially for libraries. |
We want to keep equality as it is in runes mode for multiple reasons including performance and future features such as error boundaries and async state. |
I know I'm not in the maintainer's guild for Svelte 🙂, but as an enthusiastic user, and an engineer with decent experience working on decently large systems, I'm very excited by let object = $state.raw({ x: 1 }); // reassigning (but not mutating) `object` will cause updates
let array = $state.shallow([object]); // we can push/pop/etc, but the members of the array are untouched landing. Those are features I've already been looking for in Svelte 5, and have cobbled together workarounds for. I actually have a blog post pending on FrontendMasters about these very things, and I hate the complexity I had to show to achieve those basic features (which exist in other solutions like MobX). Using something like $state.shallow() for a reactive array of non-reactive objects is an incredibly common use case. It's wasteful to set up deeply reactive plumbing for objects you know will not be changing. But here's the thing: I would not be looking for strong guarantees of immutability. I merely want reactivity to NOT be set up for all those individual properties. Most importantly, if I DO want strong guarantees of immutability, I'm free to just pull an immutable js library off the shelf, and stick a truly immutable array right inside of $state.shallow (or actually, I believe I'd use $state.raw if I understand Rich's proposal correctly). |
ส่งจากแท็บเล็ต Huawei ของฉัน-------- ข้อความต้นฉบับ --------จาก: Adam Rackis ***@***.***>วันที่: จ. 5 ส.ค. 2024 03:13ถึง: sveltejs/svelte ***@***.***>สำเนา: Subscribed ***@***.***>เรื่อง: Re: [sveltejs/svelte] Svelte 5: decide on behaviour of `$state.frozen` (Issue #12434)
I know I'm not in the maintainer's guild for Svelte 🙂, but as an enthusiastic user, and an engineer with decent experience working on decently large systems, I'm very excited by
let object = $state.raw({ x: 1 }); // reassigning (but not mutating) `object` will cause updates
let array = $state.shallow([object]); // we can push/pop/etc, but the members of the array are untouched
landing. Those are features I've already been looking for in Svelte 5, and have cobbled together workarounds for. I actually have a blog post pending on FrontendMasters about these very things, and I hate the complexity I had to show to achieve those basic features (which exist in other solutions like MobX).
Using something like $state.shallow() for a reactive array of non-reactive objects is an incredibly common use case. It's wasteful to set up deeply reactive plumbing for objects you know will not be changing. But here's the thing: I would not be looking for strong guarantees of immutability. I merely want reactivity to NOT be set up for all those individual properties.
Most importantly, if I DO want strong guarantees of immutability, I'm free to just pull an immutable js library off the shelf, and stick a truly immutable array right inside of $state.shallow (or actually, I believe I'd use $state.raw if I understand Rich's proposal correctly).
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
If you are mutating the raw object, are you happy for the UI to not update? |
Yes. I subscribed to that when I chose to use $state.shallow or $state.raw. That's the contract. That's the api. The binding itself is reactive, but the underlying values are not. If I choose to mutate them, that's on me. If you all want to go the extra mile and rig up some warnings for when I do choose to edit properties that are non-reactive that would of course be nice to have, but certainly not required imo. |
@dummdidumm I may have misunderstood Rich, but I think it's explicitly designed (only) for arrays (as opposed to "only really being useful for arrays"). It's a common enough use case that MobX has an identically named api for it Special casing $state.raw to behave differently when passed an array feels like a bit of a code smell. Avoiding different behaviors based on the argument passed to $state (or a helper) feels like the reasoning behind disallowing Lastly, |
Describe the problem
The discussion in #12413 revealed some differences of opinion around
$state.frozen
. As of that PR, it will error if passed existing reactive state, which eliminates one source of confusion. But others remain:Object.freeze
, but in prod it won't (for perf reasons). As such, the name is a bit of a misnomerObject.freeze
is so-called for a reason — it freezes the object passed in; it has a side-effect.frozen
implies that it returns a frozen object, rather than freezing its argument as a side-effect. Which isn't to say that$state.freeze
would be a better name, since that implies that it freezes existing state proxiesDescribe the proposed solution
Together, these observations suggest (to me at least) we should change the name and the behaviour. Taking a cue from MobX, perhaps
$state.ref
would be a better name.$state.raw
is another option. Doubtless there are others.This would have similar semantics to
$state.frozen
today — in other words only reassigning (not mutating) will cause updates (or in implementation terms: it creates a source but not a proxy) — but without the dev-timeObject.freeze
.Of course, by doing this we lose the opportunity to tell people that they're incorrectly mutating an object. If we want to keep that but also have consistency between dev and prod, then the alternative is to always use
Object.freeze
, perf implications be damned.Importance
nice to have
The text was updated successfully, but these errors were encountered: