🦋 Changeset detected

Latest commit:8ec1c9f

The changes in this PR will be included in the next version bump.

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

The types aren't perfect: it's possible to pass non-cloneable objects tosnapshot (likeURL), and I can't figure out a way to exclude prototype properties from the type of the clone — i.e. in a case like this...

classFoo{x=1;y=$state(2);}constsnapshot=$state.snapshot(newFoo());

...the type ofsnapshot will be this, even though the object will not have ay property:

{  x:number;  readonlyy:number;}

You also don't get a type error if you pass in an uncloneable object. We might just have to live with these limitations. On the positive side, it correctly converts e.g.SvelteSet toSet.

Ugh, the types are okay for thesnapshot function itself, but of course they don't magically apply to$state.snapshot, and it's not possible to import theSnapshot type inambient.d.ts...

Apart from whether or not we need to expose the snapshot type: Ithink it could work if you dofunction snapshot (..): import("svelte").Snapshot

Ah, you're totally right. I lean towardsnot exposing it until a need arises though, especially since it's not 100% correct

import("svelte").Snapshot

I foresee this 👀

I think we should rename this API to$state.clone now that it's not really making a snapshot anymore. We could keep the existing logic for$state.snapshot and deprecate the API and say it will be removed, to reduce friction maybe.

Why are we better off with throwing an error? Can't we just leave the state alone? Why would the user of$state.frozen care about that? They would use it in accordance to the$state.frozen contract and everything just works.

Why are we better off with throwing an error? Can't we just leave the state alone? Why would the user of$state.frozen care about that? They would use it in accordance to the$state.frozen contract and everything just works.

If you freeze the proxied object, it won't do anything and can just generally cause strange issues all over. Good read:https://tvcutsem.github.io/frozen-proxies

Can't we just leave the state alone?

There are two possibilities — we freeze the argument, or we don't. If we freeze it, we're freezing the original state which will do strange things. If we don't, then we're literally just returning the argument, which is pointless and indicates that the user messed up somehow.

Throwing an error and being clear about expectations circumvents the whole mess

We're already no longer freezing the object in prod for performance reasons, instead there's just a symbol marker on the object. If we skip freezing in dev as well (which would be good for consistency anyway - people could rely on the frozen behavior and be in for a surprise in prod), then we don't have any of those problems.

Right but do we addSTATE_FROZEN_SYMBOL to the argument even if it's already a reactive state proxy? If we do, it will have weird outcomes. If we don't, it will have weird outcomes. The only way to avoid weird outcomes is to disallow it!

We're already no longer freezing the object in prod for performance reasons, instead there's just a symbol marker on the object. If we skip freezing in dev as well (which would be good for consistency anyway - people could rely on the frozen behavior and be in for a surprise in prod), then we don't have any of those problems.

Then people will just mutate the object. I don’t see what’s wrong with the error.

Then people will just mutate the object.

I mean "rely on it" in the sense of having aObject.isFrozen check, which istrue in dev andfalse in prod.

If we don't, it will have weird outcomes

What will be weird about it? People use it like an object that is immutable, and possibly in other places it's used differently. Both can be ignorant of the other side. Nothing weird coming out of that from my perspective. I'd be fine with a dev time warning though since it's most likely an unintended mistake - but it doesn't have to be: maybe you can't really control what you get passed since you're getting part of the data from elsewhere.

weird outcomes scenario 1: we freeze the argument

<!-- Thing.svelte --><script>let { object }=$props();// one frozen object please mr svelte!let frozen=$state.frozen(object);</script>

<script>importThingfrom'./Thing.svelte';let object=$state({...});functionupdate() {object.x+=1;// hey wtf why didn't this work????  }</script><Thing {object} /><buttononclick={update}>update</button>

weird outcomes scenario 2: we don't freeze the argument

<!-- Thing.svelte --><script>importOtherThingfrom'./OtherThing.svelte';let { object }=$props();// sure am glad this object is frozen so that `<OtherThing>` can't muck about with it!let frozen=$state.frozen(object);</script><OtherThing {frozen} />

<!-- OtherThing.svelte --><script>let { frozen }=$props();frozen.x+=1;// :trollface:</script>

There's no right answer between these two. We can have extensive documentation detailing all the counterintuitive ways$state.frozen will behave in these situations, or we can just... disallow it

Again, I'm saying "stop freezing the object in dev mode because we're already not doing that in prod". That only leaves scenario 2, and mutating something does not hurt at all there. It has no practical outcome on the one freezing the object.

I'm not gonna die on this hill but my rule is "avoid throwing errors if there's sensible (fallback) behavior", and in this case that's doable for me - throwing always feels like a cop-out to me.

mutating something does not hurt at all there

In the example above, ifOtherThing.svelte mutatesfrozen it will cause updates to that object in other places, against the wishes ofThing.svelte. Unless of course we reassignfrozen...

<!-- Thing.svelte --><script>  import OtherThing from './OtherThing.svelte';  let { object } = $props();  // sure am glad this object is frozen so that `<OtherThing>` can't muck about with it!  let frozen = $state.frozen(object);+ function update_frozen(value) {+   frozen = value;+ }</script><OtherThing {frozen} />

...in which case mutations insideOtherThing.sveltewon't cause updates.

The entire purpose of$state.frozen({...}) is to not have properties be reactive. If they'realready reactive, then$state.frozen isn't doing anything except ensuring confusing behaviour if the value is later reassigned. If someone writes this...

leta=$state({...});letb=$state.frozen(a);

...then weknow, without any doubt whatsoever, thatthey fucked up. There is no "sensible (fallback) behavior". Throwing an error here is our responsibility.

(The question of whether or not we useObject.freeze in dev is separate. I can see arguments for all three options — always freeze, never freeze, or the status quo — but if we decide not to freeze in dev then we should probably call the API something different.)

If I were to use$state.frozen it wouldn't be to ensure it is never mutated, it is more about perf and/or knowing it's not proxified by me. If it already was proxified then I possibly can't control that, hence my suggestion to only issue a warning. Furthermore, the "is it not proxified"/"is it frozen" behavior only applies one level deep, so it's not really guaranteed anyway.

If we stopped freezing and called it$state.raw would that change any of your expectations of this to lean more towards my arguments?

As I said I'm not going to die on this hill, so if these arguments don't convince you then go ahead with the error.

If we stopped freezing and called it$state.raw would that change any of your expectations of this to lean more towards my arguments?

I'd have the same expectations aroundfrozen.x += 1 not causing{frozen.x} to update, and around reassignments tofrozen not behaving differently to the original declaration. But I would no longer expect the object to be frozen withObject.freeze.

So I'm definitely open to that. If we wanted totake a leaf out of MobX's book (since their terminology is pretty well established, and well understood by many people) we would call it$state.ref instead of$state.raw.

But in the meantime I'll merge this since it has one approval and you're at peace with it, so that we lock in thesnapshot improvements. We can look into the question of whether$state.frozen should stop freezing in dev and be renamed to something else as a follow-up.