extract `onchange` callbacks from options (#15579)

* WIP

* extract onchange callbacks

* const

* tweak

* docs

* fix: unwrap args in case of spread

* fix: revert unwrap args in case of spread

---------

Co-authored-by: paoloricciuti <ricciutipaolo@gmail.com>

packages/svelte/src/compiler/phases/3-transform/client/visitors/ClassBody.js

@@ -6,6 +6,7 @@ import * as b from '../../../../utils/builders.js';
 import { regex_invalid_identifier_chars } from '../../../patterns.js';
 import { get_rune } from '../../../scope.js';
 import { should_proxy } from '../utils.js';
+import { get_onchange } from './shared/state.js';
 
 /**
  * @param {ClassBody} node
@@ -117,13 +118,16 @@ export function ClassBody(node, context) {
 					);
 
 					if (field.kind === 'state' || field.kind === 'raw_state') {
-						let arg = definition.value.arguments[1];
-						let options = arg && /** @type {Expression} **/ (context.visit(arg, child_state));
+						const onchange = get_onchange(
+							/** @type {Expression} */ (definition.value.arguments[1]),
+							// @ts-ignore mismatch between Context and ComponentContext. TODO look into
+							context
+						);
 
 						value =
 							field.kind === 'state' && should_proxy(init, context.state.scope)
-								? b.call('$.assignable_proxy', init, options)
-								: b.call('$.state', init, options);
+								? b.call('$.assignable_proxy', init, onchange)
+								: b.call('$.state', init, onchange);
 					} else {
 						value = b.call('$.derived', field.kind === 'derived_by' ? init : b.thunk(init));
 					}

packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js

@@ -8,6 +8,7 @@ import * as assert from '../../../../utils/assert.js';
 import { get_rune } from '../../../scope.js';
 import { get_prop_source, is_prop_source, is_state_source, should_proxy } from '../utils.js';
 import { is_hoisted_function } from '../../utils.js';
+import { get_onchange } from './shared/state.js';
 
 /**
  * @param {VariableDeclaration} node
@@ -117,26 +118,26 @@ export function VariableDeclaration(node, context) {
 
 			const args = /** @type {CallExpression} */ (init).arguments;
 			const value = args.length > 0 ? /** @type {Expression} */ (context.visit(args[0])) : b.void0;
-			let options =
-				args.length === 2 ? /** @type {Expression} */ (context.visit(args[1])) : undefined;
+
+			const onchange = get_onchange(/** @type {Expression} */ (args[1]), context);
 
 			if (rune === '$state' || rune === '$state.raw') {
 				/**
 				 * @param {Identifier} id
 				 * @param {Expression} value
-				 * @param {Expression} [options]
+				 * @param {Expression} [onchange]
 				 */
-				const create_state_declarator = (id, value, options) => {
+				const create_state_declarator = (id, value, onchange) => {
 					const binding = /** @type {Binding} */ (context.state.scope.get(id.name));
 					const proxied = rune === '$state' && should_proxy(value, context.state.scope);
 					const is_state = is_state_source(binding, context.state.analysis);
 
 					if (proxied) {
-						return b.call(is_state ? '$.assignable_proxy' : '$.proxy', value, options);
+						return b.call(is_state ? '$.assignable_proxy' : '$.proxy', value, onchange);
 					}
 
 					if (is_state) {
-						return b.call('$.state', value, options);
+						return b.call('$.state', value, onchange);
 					}
 
 					return value;
@@ -144,7 +145,7 @@ export function VariableDeclaration(node, context) {
 
 				if (declarator.id.type === 'Identifier') {
 					declarations.push(
-						b.declarator(declarator.id, create_state_declarator(declarator.id, value, options))
+						b.declarator(declarator.id, create_state_declarator(declarator.id, value, onchange))
 					);
 				} else {
 					const tmp = context.state.scope.generate('tmp');
@@ -157,7 +158,7 @@ export function VariableDeclaration(node, context) {
 							return b.declarator(
 								path.node,
 								binding?.kind === 'state' || binding?.kind === 'raw_state'
-									? create_state_declarator(binding.node, value, options)
+									? create_state_declarator(binding.node, value, onchange)
 									: value
 							);
 						})

packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/state.js

@@ -0,0 +1,31 @@
+/** @import { Expression, Property } from 'estree' */
+/** @import { ComponentContext } from '../../types' */
+import * as b from '../../../../../utils/builders.js';
+
+/**
+ * Extract the `onchange` callback from the options passed to `$state`
+ * @param {Expression} options
+ * @param {ComponentContext} context
+ * @returns {Expression | undefined}
+ */
+export function get_onchange(options, context) {
+	if (!options) return;
+
+	if (options.type === 'ObjectExpression') {
+		const onchange = /** @type {Property | undefined} */ (
+			options.properties.find(
+				(property) =>
+					property.type === 'Property' &&
+					!property.computed &&
+					property.key.type === 'Identifier' &&
+					property.key.name === 'onchange'
+			)
+		);
+
+		if (!onchange) return;
+
+		return /** @type {Expression} */ (context.visit(onchange.value));
+	}
+
+	return b.member(/** @type {Expression} */ (context.visit(options)), 'onchange');
+}

packages/svelte/src/internal/client/index.js

@@ -113,15 +113,7 @@ export {
 	user_effect,
 	user_pre_effect
 } from './reactivity/effects.js';
-export {
-	mutable_source,
-	mutate,
-	set,
-	state,
-	update,
-	update_pre,
-	get_options
-} from './reactivity/sources.js';
+export { mutable_source, mutate, set, state, update, update_pre } from './reactivity/sources.js';
 export {
 	prop,
 	rest_props,

packages/svelte/src/internal/client/proxy.js

@@ -28,47 +28,33 @@ function identity(fn) {
 	return fn;
 }
 
-/**
- * @param {ValueOptions | undefined} options
- * @returns {ValueOptions | undefined}
- */
-function clone_options(options) {
-	return options != null
-		? {
-				onchange: options.onchange
-			}
-		: undefined;
-}
-
 /** @type {ProxyMetadata | null} */
 var parent_metadata = null;
 
 /**
  * @template T
  * @param {T} value
- * @param {ValueOptions} [_options]
+ * @param {() => void} [onchange]
  * @param {Source<T>} [prev] dev mode only
  * @returns {T}
  */
-export function proxy(value, _options, prev) {
+export function proxy(value, onchange, prev) {
 	// if non-proxyable, or is already a proxy, return `value`
 	if (typeof value !== 'object' || value === null) {
 		return value;
 	}
 
-	var options = clone_options(_options);
-
 	if (STATE_SYMBOL in value) {
 		// @ts-ignore
-		value[PROXY_ONCHANGE_SYMBOL](options?.onchange);
+		value[PROXY_ONCHANGE_SYMBOL](onchange);
 		return value;
 	}
 
-	if (options?.onchange) {
+	if (onchange) {
 		// if there's an onchange we actually store that but override the value
 		// to store every other onchange that new proxies might add
-		var onchanges = new Set([options.onchange]);
-		options.onchange = () => {
+		var onchanges = new Set([onchange]);
+		onchange = () => {
 			for (let onchange of onchanges) {
 				onchange();
 			}
@@ -116,10 +102,7 @@ export function proxy(value, _options, prev) {
 	if (is_proxied_array) {
 		// We need to create the length source eagerly to ensure that
 		// mutations to the array are properly synced with our proxy
-		sources.set(
-			'length',
-			source(/** @type {any[]} */ (value).length, clone_options(options), stack)
-		);
+		sources.set('length', source(/** @type {any[]} */ (value).length, onchange, stack));
 	}
 
 	/** @type {ProxyMetadata} */
@@ -165,12 +148,12 @@ export function proxy(value, _options, prev) {
 			var s = sources.get(prop);
 
 			if (s === undefined) {
-				s = with_parent(() => source(descriptor.value, clone_options(options), stack));
+				s = with_parent(() => source(descriptor.value, onchange, stack));
 				sources.set(prop, s);
 			} else {
 				set(
 					s,
-					with_parent(() => proxy(descriptor.value, options))
+					with_parent(() => proxy(descriptor.value, onchange))
 				);
 			}
 
@@ -184,7 +167,7 @@ export function proxy(value, _options, prev) {
 				if (prop in target) {
 					sources.set(
 						prop,
-						with_parent(() => source(UNINITIALIZED, clone_options(options), stack))
+						with_parent(() => source(UNINITIALIZED, onchange, stack))
 					);
 				}
 			} else {
@@ -201,7 +184,7 @@ export function proxy(value, _options, prev) {
 				// when we delete a property if the source is a proxy we remove the current onchange from
 				// the proxy `onchanges` so that it doesn't trigger it anymore
 				if (typeof s.v === 'object' && s.v !== null && STATE_SYMBOL in s.v) {
-					s.v[PROXY_ONCHANGE_SYMBOL](options?.onchange, true);
+					s.v[PROXY_ONCHANGE_SYMBOL](onchange, true);
 				}
 				set(s, UNINITIALIZED);
 				update_version(version);
@@ -227,18 +210,14 @@ export function proxy(value, _options, prev) {
 					// we either add or remove the passed in value
 					// to the onchanges array or we set every source onchange
 					// to the passed in value (if it's undefined it will make the chain stop)
-					if (options?.onchange != null && value && !remove) {
+					if (onchange != null && value && !remove) {
 						onchanges?.add?.(value);
-					} else if (options?.onchange != null && value) {
+					} else if (onchange != null && value) {
 						onchanges?.delete?.(value);
 					} else {
-						options = {
-							onchange: value
-						};
+						onchange = value;
 						for (let [, s] of sources) {
-							if (s.o) {
-								s.o.onchange = value;
-							}
+							s.o = value;
 						}
 					}
 				};
@@ -249,7 +228,7 @@ export function proxy(value, _options, prev) {
 
 			// create a source, but only if it's an own property and not a prototype property
 			if (s === undefined && (!exists || get_descriptor(target, prop)?.writable)) {
-				let opt = clone_options(options);
+				let opt = onchange;
 				s = with_parent(() =>
 					source(proxy(exists ? target[prop] : UNINITIALIZED, opt), opt, stack)
 				);
@@ -281,7 +260,7 @@ export function proxy(value, _options, prev) {
 
 			if (
 				is_proxied_array &&
-				options?.onchange != null &&
+				onchange != null &&
 				array_methods.includes(/** @type {string} */ (prop))
 			) {
 				return batch_onchange(v);
@@ -330,7 +309,7 @@ export function proxy(value, _options, prev) {
 				(active_effect !== null && (!has || get_descriptor(target, prop)?.writable))
 			) {
 				if (s === undefined) {
-					let opt = clone_options(options);
+					let opt = onchange;
 					s = with_parent(() => source(has ? proxy(target[prop], opt) : UNINITIALIZED, opt, stack));
 					sources.set(prop, s);
 				}
@@ -362,14 +341,14 @@ export function proxy(value, _options, prev) {
 								other_s.v !== null &&
 								STATE_SYMBOL in other_s.v
 							) {
-								other_s.v[PROXY_ONCHANGE_SYMBOL](options?.onchange, true);
+								other_s.v[PROXY_ONCHANGE_SYMBOL](onchange, true);
 							}
 							set(other_s, UNINITIALIZED);
 						} else if (i in target) {
 							// If the item exists in the original, we need to create a uninitialized source,
 							// else a later read of the property would result in a source being created with
 							// the value of the original item at that index.
-							other_s = with_parent(() => source(UNINITIALIZED, clone_options(options), stack));
+							other_s = with_parent(() => source(UNINITIALIZED, onchange, stack));
 							sources.set(i + '', other_s);
 						}
 					}
@@ -381,7 +360,7 @@ export function proxy(value, _options, prev) {
 				// object property before writing to that property.
 				if (s === undefined) {
 					if (!has || get_descriptor(target, prop)?.writable) {
-						const opt = clone_options(options);
+						const opt = onchange;
 						s = with_parent(() => source(undefined, opt, stack));
 						set(
 							s,
@@ -394,11 +373,11 @@ export function proxy(value, _options, prev) {
 					// when we set a property if the source is a proxy we remove the current onchange from
 					// the proxy `onchanges` so that it doesn't trigger it anymore
 					if (typeof s.v === 'object' && s.v !== null && STATE_SYMBOL in s.v) {
-						s.v[PROXY_ONCHANGE_SYMBOL](options?.onchange, true);
+						s.v[PROXY_ONCHANGE_SYMBOL](onchange, true);
 					}
 					set(
 						s,
-						with_parent(() => proxy(value, clone_options(options)))
+						with_parent(() => proxy(value, onchange))
 					);
 				}
 			})();
@@ -464,11 +443,11 @@ export function proxy(value, _options, prev) {
 /**
  * @template T
  * @param {T} value
- * @param {ValueOptions} [options]
+ * @param {() => void} [onchange]
  * @returns {Source<T>}
  */
-export function assignable_proxy(value, options) {
-	return state(proxy(value, options), options);
+export function assignable_proxy(value, onchange) {
+	return state(proxy(value, onchange), onchange);
 }
 
 /**

packages/svelte/src/internal/client/reactivity/sources.js

@@ -77,7 +77,7 @@ export function batch_onchange(fn) {
 /**
  * @template V
  * @param {V} v
- * @param {ValueOptions} [o]
+ * @param {() => void} [o]
  * @param {Error | null} [stack]
  * @returns {Source<V>}
  */
@@ -102,18 +102,10 @@ export function source(v, o, stack) {
 	return signal;
 }
 
-/**
- * @param {Source} source
- * @returns {ValueOptions | undefined}
- */
-export function get_options(source) {
-	return source.o;
-}
-
 /**
  * @template V
  * @param {V} v
- * @param {ValueOptions} [o]
+ * @param {() => void} [o]
  * @param {Error | null} [stack]
  */
 export function state(v, o, stack) {
@@ -196,11 +188,11 @@ export function internal_set(source, value) {
 	if (!source.equals(value)) {
 		var old_value = source.v;
 
-		if (typeof old_value === 'object' && old_value != null && source.o?.onchange) {
+		if (typeof old_value === 'object' && old_value != null && source.o) {
 			// @ts-ignore
 			const remove = old_value[PROXY_ONCHANGE_SYMBOL];
 			if (remove && typeof remove === 'function') {
-				remove(source.o?.onchange, true);
+				remove(source.o, true);
 			}
 		}
 
@@ -257,7 +249,7 @@ export function internal_set(source, value) {
 			inspect_effects.clear();
 		}
 
-		var onchange = source.o?.onchange;
+		var onchange = source.o;
 		if (onchange) {
 			if (onchange_batch) {
 				onchange_batch.add(onchange);

packages/svelte/src/internal/client/reactivity/types.d.ts

@@ -20,8 +20,8 @@ export interface Value<V = unknown> extends Signal {
 	rv: number;
 	/** The latest value for this signal */
 	v: V;
-	/** Options for the source */
-	o?: ValueOptions;
+	/** onchange callback */
+	o?: () => void;
 	/** Dev only */
 	created?: Error | null;
 	updated?: Error | null;