breaking: finer lazy reactivity for set by Azarattum · Pull Request #11287 · sveltejs/svelte

Azarattum · 2024-04-22T09:33:01Z

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 · 2024-04-22T09:33:05Z

⚠️ 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 · 2024-04-22T09:33:36Z

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

Azarattum · 2024-04-22T09:34:15Z

@paoloricciuti, you might be interested in this

paoloricciuti · 2024-04-22T13:37:03Z

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 · 2024-04-22T13:46:57Z

@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 · 2024-04-22T15:46:55Z

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

FoHoOV · 2024-04-25T17:33:26Z

@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 · 2024-04-25T18:42:42Z

@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 · 2024-04-26T19:11:17Z

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 · 2024-04-26T19:17:01Z

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 · 2024-04-30T05:00:25Z

#11385

trueadm · 2024-04-30T11:47:16Z

packages/svelte/src/reactivity/set.js

+			this.#tracked.set(value, s);
 		}

+		effect(() => () => {


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

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

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

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

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

Azarattum · 2024-05-09T04:20:23Z

Can someone familiar with Svelte internals help refactor this part?

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

@trueadm pointed out that creating an effect here could be a performance concern, even though we don't use the effect itself only the cleanup logic. Is there a more direct way to add code to the parent's effect cleanup function without creating a nested effect?

trueadm · 2024-05-09T13:52:13Z

@Azarattum We need to avoid coding patterns that need a cleanup stage altogether really. Sets can be created outside of effects too, and these objects need to be robust enough to work in those cases without leaking too.

Azarattum · 2024-05-10T03:48:54Z

@trueadm good point. If .has is called outside of an effect we should just return the value without creating any signals or nested effects.

Do you think that adding an early return would make the code viable? Or would we still need to refactor the cleanup logic somehow?

trueadm · 2024-05-10T20:14:57Z

@Azarattum I'd prefer if we just avoided any need for cleanup entirely, so a refactor would be needed. It might make more sense to implement the individual methods one by one and then coming up with utility functions to handle the common cases.

FoHoOV · 2024-05-12T05:25:36Z

@Azarattum I'd prefer if we just avoided any need for cleanup entirely, so a refactor would be needed. It might make more sense to implement the individual methods one by one and then coming up with utility functions to handle the common cases.

@trueadm You might be interested in #11504

Rich-Harris · 2024-06-08T19:14:47Z

superseded by #11967, so i'll close this — thanks

breaking: finer lazy reactivity for set

d2063ca

Azarattum mentioned this pull request Apr 22, 2024

Svelte5: reactive Set constructor not working correctly #11222

Closed

Azarattum added 2 commits April 23, 2024 08:18

fix: defer call to has till needed

69bae17

chore: regenerate and improve set typings

3775935

Azarattum mentioned this pull request Apr 30, 2024

fix: ensure no data loss occurs when using reactive Set methods #11385

Merged

5 tasks

trueadm reviewed Apr 30, 2024

View reviewed changes

FoHoOV mentioned this pull request May 7, 2024

feat: generic utility for making any class reactive #11504

Closed

9 tasks

Azarattum mentioned this pull request May 9, 2024

svelte 5 : with reactive set has invokes in effects where it shouldn't #11515

Closed

Azarattum mentioned this pull request May 31, 2024

fix: improve reactive Map and Set implementations #11827

Merged

Rich-Harris closed this Jun 8, 2024

Uh oh!

Conversation

Azarattum commented Apr 22, 2024

Svelte 5 rewrite

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

Tests and linting

Uh oh!

changeset-bot bot commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

Azarattum commented Apr 22, 2024

Uh oh!

Azarattum commented Apr 22, 2024

Uh oh!

paoloricciuti commented Apr 22, 2024

Uh oh!

Azarattum commented Apr 22, 2024

Uh oh!

paoloricciuti commented Apr 22, 2024

Uh oh!

FoHoOV commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paoloricciuti commented Apr 25, 2024

Uh oh!

FoHoOV commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paoloricciuti commented Apr 26, 2024

Uh oh!

hakoomen commented Apr 30, 2024

Uh oh!

trueadm Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

Azarattum Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

trueadm Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

Azarattum Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

Azarattum Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

Azarattum commented May 9, 2024

Uh oh!

trueadm commented May 9, 2024

Uh oh!

Azarattum commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trueadm commented May 10, 2024

Uh oh!

FoHoOV commented May 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rich-Harris commented Jun 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

changeset-bot bot commented Apr 22, 2024 •

edited

Loading

FoHoOV commented Apr 25, 2024 •

edited

Loading

FoHoOV commented Apr 26, 2024 •

edited

Loading

Azarattum commented May 10, 2024 •

edited

Loading

FoHoOV commented May 12, 2024 •

edited

Loading