breaking: use structuredClone inside $state.snapshot

[Rich-Harris]

fixes #12128 (and possibly others, unsure)

We previously decided that $state.snapshot should only de-proxify its argument, and whatever state it contained. In other words in a case like this...

const obj = $state({...});

console.log($state.snapshot(obj));
console.log($state.snapshot({ obj }));

...the first snapshot would be de-proxified but the second one wouldn't.

This was a bad call. It's wantonly confusing, prone to surprising breakage, and doesn't work for common scenarios like serializing classes with state fields. We also don't clone objects that aren't state proxies, meaning that if you mutate parts of a snapshot it will mutate the underlying object, and vice versa — the opposite of what 'snapshot' implies.

The approach in this PR is a lot more robust. It's basically structuredClone with a couple of edits:

• state proxies are de-proxified (since structuredClone balks at proxies)
• if a class has a toJSON method, we use it

A few consequences of this design:

• we no longer preserve non-enumerable properties, or descriptors more generally — just like with structuredClone normally, if you have an object with a getter, the getter is invoked. This is deliberate, because interacting with a getter/setter is likely to change the underlying state, which is undesirable. Functions don't get cloned, and there's no reason accessors should be exempt from that rule
• Changing parts of the snapshot won't affect the original state, and vice versa. This extends to things like maps and sets
• Instances of svelte/reactivity classes are turned into their non-reactive counterparts — e.g. a snapshotted SvelteSet becomes a normal Set (not because of any special handling, but just because that's how structuredClone works when encountering an object where instanceof Set is true)
• Userland classes are turned into POJOs, including classes with state fields. Note that state fields are non-enumerable, so if you have a state field that you want to snapshot, you should implement toJSON on the class
• It's slower, and that's okay. $state.snapshot isn't something you should be using in a hot code path. This is a case where correctness and predictability are far more important than eking out a few extra microseconds

At present, the types are wrong. I suspect it'll take some trickery to make them behave correctly. Will take a swing at it but might need to call @dummdidumm for backup...

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: 8ec1c9f

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

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

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

[Rich-Harris]

The types aren't perfect: it's possible to pass non-cloneable objects to snapshot (like URL), 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...

class Foo {
  x = 1;
  y = $state(2);
}

const snapshot = $state.snapshot(new Foo());

...the type of snapshot will be this, even though the object will not have a y property:

{
  x: number;
  readonly y: 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 to Set.

[Rich-Harris]

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

[dummdidumm]

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

[Rich-Harris]

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

[PuruVJ]

import("svelte").Snapshot

I foresee this 👀

[trueadm]

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.

[dummdidumm]

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.

[trueadm]

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

[Rich-Harris]

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

[dummdidumm]

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.

[Rich-Harris]

Right but do we add STATE_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!

[trueadm]

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.

[dummdidumm]

Then people will just mutate the object.

I mean "rely on it" in the sense of having a Object.isFrozen check, which is true in dev and false 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.

[Rich-Harris]

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>
  import Thing from './Thing.svelte';
  let object = $state({...});

  function update() {
    object.x += 1; // hey wtf why didn't this work????
  }
</script>

<Thing {object} />
<button onclick={update}>update</button>

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

<!-- 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);
</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

[dummdidumm]

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.

[Rich-Harris]

mutating something does not hurt at all there

In the example above, if OtherThing.svelte mutates frozen it will cause updates to that object in other places, against the wishes of Thing.svelte. Unless of course we reassign frozen...

<!-- 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 inside OtherThing.svelte won't cause updates.

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

let a = $state({...});
let b = $state.frozen(a);

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

[Rich-Harris]

(The question of whether or not we use Object.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.)

[dummdidumm]

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.

[Rich-Harris]

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 around frozen.x += 1 not causing {frozen.x} to update, and around reassignments to frozen not behaving differently to the original declaration. But I would no longer expect the object to be frozen with Object.freeze.

So I'm definitely open to that. If we wanted to take 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 the snapshot 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.