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

$state.snapshot cause object prototype to be lost #15022

Open
codercms opened this issue Jan 15, 2025 · 14 comments
Open

$state.snapshot cause object prototype to be lost #15022

codercms opened this issue Jan 15, 2025 · 14 comments

Comments

@codercms
Copy link

Describe the bug

There's a doc on an official website:
To take a static snapshot of a deeply reactive $state proxy, use $state.snapshot:

<script>
	let counter = $state({ count: 0 });

	function onclick() {
		// Will log `{ count: ... }` rather than `Proxy { ... }`
		console.log($state.snapshot(counter));
	}
</script>

This is handy when you want to pass some state to an external library or API that doesn’t expect a proxy, such as structuredClone.

But actually $state.snapshot doesn't work in cases when an external library expects to receive an instance of certain class, not just a regular object. Cause $state.snapshot actually makes a copy of object using window.structuredClone API which doesn't keep object prototype, nor private properties: Things that don't work with structured clone .

Which makes usage of $state.snapshot completely broken with libraries that actually work with classes, not an "anonymous" objects.

Reproduction

Here's a minimal code for reproduction:

<script lang="ts" module>
	class MyObject {
		sayHello() {
			console.log('Hello!');
		}
	}
</script>

<script lang="ts">
	import { onMount } from 'svelte';

	let someState = $state(new MyObject());
	let someSnapshot = $state.snapshot(someState);

	console.log('Prototype', { original: Object.getPrototypeOf(someState), snapshot: Object.getPrototypeOf(someSnapshot) });

	onMount(() => {
		someState.sayHello();
		someSnapshot.sayHello();
	})
</script>

Another scenario:

There's an awesome headless UI library called Zag, which has date-picker component
Minimal code to use it with Svelte is:

<script lang="ts">
  import * as datepicker from "@zag-js/date-picker"
  import { portal, useMachine, normalizeProps } from "@zag-js/svelte"

  const [snapshot, send] = useMachine(datepicker.machine({ id: "1" }))
  const api = $derived(datepicker.connect(snapshot, send, normalizeProps))
</script>

But what if I want to create a simple component that have some reactive props and passes them down to Zag?
So my minimal code will look like this:

<script lang="ts">
	import * as datepicker from '@zag-js/date-picker';
	import { portal, useMachine, normalizeProps } from '@zag-js/svelte';

	import type { DatePickerProps } from './types';
	import { useId } from '$lib/components/zag/id';

	let {
		// Zag ---
		value = $bindable([]),
		min = $bindable(),
		max = $bindable(),

		...zagProps
	}: DatePickerProps = $props();

	const [snapshot, send] = useMachine(
		datepicker.machine({
			id: useId(),
			value: value.map(val => datepicker.parse(val)),
			min: min ? datepicker.parse(min) : undefined,
			max: max ? datepicker.parse(max) : undefined,
		}),
		{
			context: {
				...zagProps,
				get value() {
					console.log('getValue triggered');

					return value.map(val => datepicker.parse(val));
				},
				get min() {
					console.log('getMin triggered');

					return min ? datepicker.parse(min) : undefined;
				},
				get max() {
					console.log('getMax triggered');

					return max ? datepicker.parse(max) : undefined;
				},
			}
		}
	);
	const api = $derived(datepicker.connect(snapshot, send, normalizeProps));

Such approach of using Zag could be found in Skeleton UI project for Svelte

Under hood Zag will make context props reactive, using $effect + $state.snapshot runes in Svelte adapter:

But Zag expects to get an instance of class CalendarDate (which has some methods, e.g. toDate, compare, etc.) for min/max and an array of instances of CalendarDate for value in this case, but once $state.snapshot is used to get context props all "type" information about min/max/value get lost.
And as a result Zag will fail with an error like: toDate is not a function

So to work-around it I'll have to create ugly things like this:

CalendarDate.prototype.toJSON = function () {
	const copiedObject = { ...this };

	copiedObject.copy = this.copy.bind(copiedObject);
	copiedObject.add = this.add.bind(copiedObject);
	copiedObject.subtract = this.subtract.bind(copiedObject);
	copiedObject.set = this.set.bind(copiedObject);
	copiedObject.cycle = this.cycle.bind(copiedObject);
	copiedObject.toDate = this.toDate.bind(copiedObject);
	copiedObject.toString = this.toString.bind(copiedObject);
	copiedObject.compare = this.compare.bind(copiedObject);

	console.log('toJSON is called', {
		copy: copiedObject,
		this: this,
	});

	return copiedObject;
}

Logs

Prototype

original: Object { … }
​​
constructor: class MyObject {}​​
sayHello: function sayHello()
...

snapshot: Object { … }
​​
__defineGetter__: function __defineGetter__()
​​
__defineSetter__: function __defineSetter__()
​​
__lookupGetter__: function __lookupGetter__()
​​
__lookupSetter__: function __lookupSetter__()
​​
__proto__: 
​​
constructor: function Object()
...

Hello!

Uncaught (in promise) TypeError: someSnapshot.sayHello is not a function

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (24) x64 13th Gen Intel(R) Core(TM) i7-13700HX
    Memory: 7.39 GB / 31.63 GB
  Binaries:
    Node: 20.18.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.9.0 - C:\Program Files\nodejs\npm.CMD
    pnpm: 9.15.3 - C:\Program Files\nodejs\pnpm.CMD
    bun: 1.0.29 - ~\.bun\bin\bun.EXE
  Browsers:
    Edge: Chromium (127.0.2651.74)
    Internet Explorer: 11.0.22621.3527

Severity

blocking all usage of svelte

@paoloricciuti
Copy link
Member

Classes are not proxied so you don't need to $state.snapshot them...the only problem that could arise is if you have a stateful object as a state of your class...is that the case for your example?

@codercms
Copy link
Author

@paoloricciuti I see your point.

There's a bit different case with Zag, here's part of Svelte Adapter - https://github.com/chakra-ui/zag/blob/b7ad1ef6d1e8f1e381bc838ba719eb7434c2c16e/packages/frameworks/svelte/src/use-service.svelte.ts

This adapter is responsive to react for Svelte reactive properties changes:

  $effect(() => {
    const contextSnapshot = $state.snapshot(options?.context)
    // @ts-expect-error - svelte typing issue
    service.setContext(contextSnapshot)
  })

And then apply changes to component's internal state.

"Context" in this case is an object that holds "public" state and you're free to change anything you want here. But context properties could contain props of literally any types: scalar types (like number, string, etc.), arrays, objects, functions and of course instances of some class.

And since whole context is passed to $state.snapshot class instances returned from any getter in context get broken

@Leonidaz
Copy link

kind of similar to #14988

@paoloricciuti @trueadm

I wonder if the clone function, should not attempt applying structuredClone if the value is of object type but doesn't match any of the current conditions inside the if (typeof value === 'object' && value !== null) block, assuming that another check is added to test if value is a known svelte state proxy via the known Symbol's.

And if it's not a svelte proxy or any of the handled object types, it just doesn't clone it and returns the value, instead of going ahead with trying structuredClone ?

If clone() is used for some other use cases, wonder if it could take a param from snapshot to cause the above behavior...

@trueadm
Copy link
Contributor

trueadm commented Jan 16, 2025

kind of similar to #14988

@paoloricciuti @trueadm

I wonder if the clone function, should not attempt applying structuredClone if the value is of object type but doesn't match any of the current conditions inside the if (typeof value === 'object' && value !== null) block, assuming that another check is added to test if value is a known svelte state proxy via the known Symbol's.

And if it's not a svelte proxy or any of the handled object types, it just doesn't clone it and returns the value, instead of going ahead with trying structuredClone ?

If clone() is used for some other use cases, wonder if it could take a param from snapshot to cause the above behavior...

That would be a breaking change. $state.snapshot is meant to take a clone. It has no guarantees around preserving the prototype. For classes that use state, and want to be able to provide accurate information, then toJSON should be used.

We'd have to provide some 2nd argument to indicate this maybe? Tricky one.

@paoloricciuti
Copy link
Member

I think there's indeed something missing here because if some library it's using snapshot then you can't do anything about it, even if you use toJSON. But I'm not really sure how we can fix this

@trueadm
Copy link
Contributor

trueadm commented Jan 16, 2025

I think there's indeed something missing here because if some library it's using snapshot then you can't do anything about it, even if you use toJSON. But I'm not really sure how we can fix this

I mean the original post mentions needing to pass this to external libraries. However, if you have a class with $state on it, and we don't clone that class, then it will still be reactive to the outside, which kind of defeats the point of the original post's intent.

@codercms
Copy link
Author

@paoloricciuti @trueadm maybe it's better to not rely on JSON serialization at all?
e.g. if an object has a method called "copy" (or any other meaningful name) then just call this method and immediately return result

@paoloricciuti
Copy link
Member

@paoloricciuti @trueadm maybe it's better to not rely on JSON serialization at all? e.g. if an object has a method called "copy" (or any other meaningful name) then just call this method and immediately return result

You copy method is just toJSON (you can return an object there) but the issue persists...either you call it copy or toJSON you will loose your methods.

@codercms
Copy link
Author

@paoloricciuti here's the part of $state.snapshot clone function:

if (typeof (/** @type {T & { toJSON?: any } } */ (value).toJSON) === 'function') {
return clone(
/** @type {T & { toJSON(): any } } */ (value).toJSON(),
cloned,
DEV ? `${path}.toJSON()` : path,
paths,
// Associate the instance with the toJSON clone
value
);
}
}

The problem with toJSON is that its result isn't immediately returned; instead, it is processed recursively by the clone function.
So actually toJSON isn't exactly what "copy" should do

@trueadm
Copy link
Contributor

trueadm commented Jan 16, 2025

The problem with toJSON is that its result isn't immediately returned; instead, it is processed recursively by the clone function. So actually toJSON isn't exactly what "copy" should do

That might be a bug. Not sure, needs an investigation.

@paoloricciuti
Copy link
Member

@paoloricciuti here's the part of $state.snapshot clone function:

svelte/packages/svelte/src/internal/shared/clone.js

Lines 102 to 112 in 973b51d

if (typeof (/** @type {T & { toJSON?: any } } / (value).toJSON) === 'function') {
return clone(
/
* @type {T & { toJSON(): any } } */ (value).toJSON(),
cloned,
DEV ? ${path}.toJSON() : path,
paths,
// Associate the instance with the toJSON clone
value
);
}
}
The problem with toJSON is that its result isn't immediately returned; instead, it is processed recursively by the clone function. So actually toJSON isn't exactly what "copy" should do

Yeah but that's what @trueadm was talking about...if you call snapshot you expect the value to not be reactive anymore so we need to recursively snapshot it

@codercms
Copy link
Author

@trueadm @paoloricciuti I see your point, but what if the clone function could be extended to allow users to control how a value is copied (not in the way like toJSON works)?

For example, when you pass an object with a "copy", "clone", or similar method to $state.snapshot, the function could prioritize this method, call it, and immediately return its result.

This would act as a form of user-land reactivity control.

@trueadm
Copy link
Contributor

trueadm commented Jan 16, 2025

@trueadm @paoloricciuti I see your point, but what if the clone function could be extended to allow users to control how a value is copied (not in the way like toJSON works)?

For example, when you pass an object with a "copy", "clone", or similar method to $state.snapshot, the function could prioritize this method, call it, and immediately return its result.

This would act as a form of user-land reactivity control.

We considered that. However, if you had a field on your class that was using $state and it was deeply reactive, then you'd need to apply $state.snapshot on that again before returning it. In practice users mess it up and end up exposing reactivity outside again, causing frustrating bugs.

Feel free to explore changing it and putting it in a PR so we can play around with it. I don't really understand how we can avoid all the problems mentioned in this thread, but maybe there's a clever way you can do it.

@Leonidaz
Copy link

It would really be great to document toJSON for classes in the $state.snapshot section: https://svelte.dev/docs/svelte/$state#$state.snapshot

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

No branches or pull requests

4 participants