breaking: finer lazy reactivity for set

[Azarattum]

Svelte 5 rewrite

Following #11200, should fix #11222.

This implementation keeps all the data in the super set while maintaining minimal data duplication. This is achieved with lazy signal initialization. In set the only fine-grained method is .has therefore we can initialize signals only when requested by it. This also has a benefit of not firing effects when a value doesn't exist, hasn't been appended, but the version has changed (which is a breaking change, but it's a good one). Meaning:

$effect(() => set.has(2));
set.add(2); // cause update
set.delete(2); // cause update
set.add(4); // won't cause an update (did before)
set.add(2); // cause update

The implementation details are subject to discussion. Especially:

effect(() => () => {
	queueMicrotask(() => {
		if (s && !s.reactions) {
			this.#tracked.delete(value);
		}
	});
});

Is there a better way to cleanup unused signals?

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]

⚠️ No Changeset found

Latest commit: 3775935

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Azarattum]

If the idea goes through, we should implement the same strategy for map

[Azarattum]

@paoloricciuti, you might be interested in this

[paoloricciuti]

So let me see if i understand this correctly: this could still duplicate all the data in the Map but in the best case it will only keep in the map the things that i'm asking for .has right? So the data duplication would still be lower than what's currently implemented and reactivity for non fine grained methods is still maintained by the version.

Did i understood that correctly?

[Azarattum]

@paoloricciuti, exactly. In the worst case it would match the current implementation, but on average should be much more memory efficient.

We also get the benefit of tracking non-existent items without a version (which results in finer updates). Also initializing signals lazily might make it even more memory efficient than the approach in #11200 (but the difference here is negligible)

[paoloricciuti]

Seems good but let's wait on some maintainers to validate. Will the Map implementation still be possible or there are unknowns?

[FoHoOV]

@Azarattum @paoloricciuti In the original issue I mentioned proxies, it has no data duplication and very easy to extend.
this is the gist of my idea, some types are wrong and implementation is not complete:

set.js

export const ReactiveSet = make_reactive(Set, {
	mutation_properties: ['add', 'clear', 'delete']
});

utils.js

/**
 * @typedef {object} Options
 * @prop {string[]} mutation_properties
 */

/**
 * @template {new (...args: any) => any} TEntity
 * @callback ReactiveEntityBuilder
 * @param {...ConstructorParameters<TEntity>} params - parameters for TEntity constructor
 * @returns {InstanceType<TEntity>}
 */

/**
 * @template {new (...args: any) => any} TEntity - the entity we want to make reactive
 * @param {TEntity} Entity - the class/function we want to make reactive
 * @param {Options} options - configurations for how reactivity works for this entity
 * @returns {ReactiveEntityBuilder<TEntity>}
 */
export const make_reactive = (Entity, options) => {
	// @ts-ignore
	return (...params) => {
		const entity_instance = new Entity(...params);
		return new Proxy(source(0), {
			get(target, property, receiver) {
				const orig_property = entity_instance[property];

				let result;

				if (typeof orig_property === 'function') {
					// Bind functions directly to the Set
					result = orig_property.bind(entity_instance);
				} else {
					// Properly handle getters
					result = Reflect.get(entity_instance, property, entity_instance);
				}

				if (options.mutation_properties.some((v) => v == property)) {
					set(target, target.v + 1);
				} else {
					get(target);
				}

				return result;
			}
		});
	};
};

If you like the idea I can continue working on it because I actually don't know how svelte5 reactivity works under the hood. This works in playground but I don't know if this the correct way to do it.

[paoloricciuti]

@Azarattum @paoloricciuti In the original issue I mentioned proxies, it has no data duplication and very easy to extend. this is the gist of my idea, some types are wrong and implementation is not complete:

set.js

export const ReactiveSet = make_reactive(Set, {
	mutation_properties: ['add', 'clear', 'delete']
});

utils.js

/**
 * @typedef {object} Options
 * @prop {string[]} mutation_properties
 */

/**
 * @template {new (...args: any) => any} TEntity
 * @callback ReactiveEntityBuilder
 * @param {...ConstructorParameters<TEntity>} params - parameters for TEntity constructor
 * @returns {InstanceType<TEntity>}
 */

/**
 * @template {new (...args: any) => any} TEntity - the entity we want to make reactive
 * @param {TEntity} Entity - the class/function we want to make reactive
 * @param {Options} options - configurations for how reactivity works for this entity
 * @returns {ReactiveEntityBuilder<TEntity>}
 */
export const make_reactive = (Entity, options) => {
	// @ts-ignore
	return (...params) => {
		const entity_instance = new Entity(...params);
		return new Proxy(source(0), {
			get(target, property, receiver) {
				const orig_property = entity_instance[property];

				let result;

				if (typeof orig_property === 'function') {
					// Bind functions directly to the Set
					result = orig_property.bind(entity_instance);
				} else {
					// Properly handle getters
					result = Reflect.get(entity_instance, property, entity_instance);
				}

				if (options.mutation_properties.some((v) => v == property)) {
					set(target, target.v + 1);
				} else {
					get(target);
				}

				return result;
			}
		});
	};
};

If you like the idea I can continue working on it because I actually don't know how svelte5 reactivity works under the hood. This works in playground but I don't know if this the correct way to do it.

Let's wait on maintainers to take a look at this: the first thing I notice is that with this ReactiveSet is not a class but a function. Another possible drawback could be for functions that expect a normal set (you might not be able to pass this to those functions in TS). This is also registering accessing the source hence creating a dependency on get rather then apply (so for example doing reactive_set.has will trigger reactivity even without calling it. Oh and most importantly this has a single signal for the whole set which could make things less fine grained.

But again let's wait on the far more experienced maintainers to take the decision, I'm just a guy lol

[FoHoOV]

Let's wait on maintainers to take a look at this: the first thing I notice is that with this ReactiveSet is not a class but a function. Another possible drawback could be for functions that expect a normal set (you might not be able to pass this to those functions in TS). This is also registering accessing the source hence creating a dependency on get rather then apply (so for example doing reactive_set.has will trigger reactivity even without calling it. Oh and most importantly this has a single signal for the whole set which could make things less fine grained.

But again let's wait on the far more experienced maintainers to take the decision, I'm just a guy lol

Ow your absoulutly correct! thanks for the callouts. Most of them can be solved though. The good thing I like about this way is that we don't need a different implementation for each builtin and not data duplication. I also updated it a bit to address some of the issues (also I've removed all types for now cuz it needs more work):

// set.js
// could be the same for other builtins
export const ReactiveSet = make_reactive(Set, {
	mutation_properties: ['add', 'clear', 'delete'],
	interceptors: {
		add: (value, property, ...params) => {
			return !value.has(params[0]);
		},
		clear: (value, property, ...params) => {
			return value.size !== 0;
		},
		delete: (value, property, ...params) => {
			return !value.has(params[0]);
		}
	}
});

// usage for now as you mentions is something like:
// const set = ReactiveSet([1, 2, 3]);
// but we can do some tricks to be able to call it with `new`
// also the return type can also be casted to `TEntity` which is `Set`, then TS won't yell

// utils.js
export const make_reactive = (Entity, options) => {
	function notify_if_required(target, property, value, ...params) {
		//  interceptors - you return true if you want to notify reactions otherwise return false
		if (options.interceptors?.[property]?.(value, property, ...params) === false) {
			// if interceptor said to not make this call reactive(by returning false) then bailout
			return;
		}

		if (options.mutation_properties.some((v) => v === property)) {
			set(target, target.v + 1);
		} else {
			get(target);
		}
	}

	// we can cast it to `()=>typeof Entity` which will trick TS into thinking this is a `Entity` (for instance a `Set`) rather than a `Source<number>`
	// and actually it isn't because the proxy will only forward whatever is in the `Entity` and nothing else, so it actually behaves exactly the same as an Entity (for instance a `Set`)
	return (...params) => {
		const entity_instance = new Entity(...params);

		return new Proxy(source(0), {
			get(target, property) {
				const orig_property = entity_instance[property];

				let result;

				if (typeof orig_property === 'function') {
					// Bind functions directly to the Set
					result = ((/** @type {any} */ ...params) => {
						// this is called when the function is actually called rather than when we access it
						// for instance if get `set.has` it will do nothing, but when we call it,  reactivity will get invoked (based on the interceptors)
						notify_if_required(target, property, entity_instance, ...params);
						return orig_property.bind(entity_instance)(...params);
					}).bind(entity_instance);
				} else {
					// Properly handle getters
					result = Reflect.get(entity_instance, property, entity_instance);
					notify_if_required(target, property, entity_instance);
				}

				return result;
			}
		});
	};
};

[paoloricciuti]

Let's wait on maintainers to take a look at this: the first thing I notice is that with this ReactiveSet is not a class but a function. Another possible drawback could be for functions that expect a normal set (you might not be able to pass this to those functions in TS). This is also registering accessing the source hence creating a dependency on get rather then apply (so for example doing reactive_set.has will trigger reactivity even without calling it. Oh and most importantly this has a single signal for the whole set which could make things less fine grained.
But again let's wait on the far more experienced maintainers to take the decision, I'm just a guy lol

Ow your absoulutly correct! thanks for the callouts. Most of them can be solved though. The good thing I like about this way is that we don't need a different implementation for each builtin. I also updated it a bit to address some of the issues (also I've removed all types for now cuz it needs more work):

// set.js
// could be the same for other builtins
export const ReactiveSet = make_reactive(Set, {
	mutation_properties: ['add', 'clear', 'delete'],
	interceptors: {
		add: (value, property, ...params) => {
			return !value.has(params[0]);
		},
		clear: (value, property, ...params) => {
			return value.size !== 0;
		},
		delete: (value, property, ...params) => {
			return !value.has(params[0]);
		}
	}
});

// usage for now as you mentions is something like:
// const set = ReactiveSet([1, 2, 3]);
// but we can do some tricks to be able to call it with `new`
// also the return type can also be casted to `TEntity` which is `Set`, then TS won't yell

// utils.js
export const make_reactive = (Entity, options) => {
	function notify_if_required(target, property, value, ...params) {
		//  interceptors - you return true if you want to notify reactions otherwise return false
		if (options.interceptors?.[property]?.(value, property, ...params) === false) {
			// if interceptor said to not make this call reactive(by returning false) then bailout
			return;
		}

		if (options.mutation_properties.some((v) => v === property)) {
			set(target, target.v + 1);
		} else {
			get(target);
		}
	}

	// we can cast it to `()=>typeof Entity` which will trick TS into thinking this is a `Entity` (for instance a `Set`) rather than a `Source<number>`
	// and actually it isn't because the proxy will only forward whatever is in the `Entity` and nothing else, so it actually behaves exactly the same as an Entity (for instance a `Set`)
	return (...params) => {
		const entity_instance = new Entity(...params);

		return new Proxy(source(0), {
			get(target, property) {
				const orig_property = entity_instance[property];

				let result;

				if (typeof orig_property === 'function') {
					// Bind functions directly to the Set
					result = ((/** @type {any} */ ...params) => {
						// this is called when the function is actually called rather than when we access it
						// for instance if get set.has it will do nothing, but when we call only then reactivity will get invoked
						notify_if_required(target, property, entity_instance, ...params);
						return orig_property.bind(entity_instance)(...params);
					}).bind(entity_instance);
				} else {
					// Properly handle getters
					result = Reflect.get(entity_instance, property, entity_instance);
					notify_if_required(target, property, entity_instance);
				}

				return result;
			}
		});
	};
};

I think you did enough work that it would be easier to just create a PR and reference this PR 😁

[hakoomen]

#11385

[trueadm]

Not sure we want to invoke an effect as part of this method. It will add considerable overhead.

[Azarattum]

Sure. I'm not that familiar with internals. Is there a better way to register a cleanup for a running effect?

[trueadm]

I'm not sure this approach of needing to clean up is the right one. Why not keep the versioning logic from before?

[Azarattum]

The versioning logic is still there for non fine-grained updates. Signals are now created only per user's request (e.g. with .has). They should be cleaned up when they are no longer used anymore, though

[Azarattum]

This is only required if .has is called from an effect. If we can somehow hook onto the parent's effect cleanup logic, that would be great