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

feat: generic utility for making any class reactive #11504

Open
wants to merge 53 commits into
base: main
Choose a base branch
from

Conversation

FoHoOV
Copy link

@FoHoOV FoHoOV commented May 7, 2024

This is a completely new take on the reactivity package. Right now, if you want to make a class reactive, you have to create a subclass and manually set up signals for each property. For classes like Set or Map, you even have to duplicate the data in an inner Set or Map, which can lead to bugs and bad memory management. This PR is inspired by #11287 and the current implementation.

Goal

  • Make any class reactive in a generic way
  • Make something that makes it easy to be as fine-grained as possible
  • No data duplication
  • Not worrying about data management. All we should care about is who notifies who and what should be reactive.

Let’s break down how it works. Every class has properties we read from and properties we write to, similar to the current implementation.

Read Properties:

  • No Parameters: Properties like set.size, map.keys(), and set.values() don’t take any parameters and should be reactive based on actual changes.
  • With Parameters: Properties like set.get(x), map.get(x), or something.someMethod(x, y) depend on one or more parameters and should be reactive based on those parameters and certain conditions.
  • Conditional Reactivity: Properties like url.origin or url.hash don’t take any parameters but are reactive based on certain conditions rather than actual changes. For example, setting url.hash doesn’t change url.origin, and vice versa.

Write Properties:

  • No Parameters: Properties like set.clear() don’t take any parameters and could cause reactivity based on some conditions or just cause reactivity with each call.
  • With Parameters: Properties like set.add(x), map.set(x, y), or url.href = x take a parameter and should cause reactivity based on that parameter.

Each write property could cause reactivity. Based on this, we have two types of reactivity inside classes:

  1. Write properties that take a parameter and cause reactivity based on some conditions:
    • Notify all read properties.
    • Notify only some read properties.
  2. Write properties that don't take a parameter and cause reactivity.
    • Notify all read properties.
    • Notify only some read properties.

With these scenarios, we can create a utility that covers everything mentioned. Here’s a code sample to explain what it does:

export const ReactiveSet = make_reactive(Set, {
    write_properties: ['add', 'clear', 'delete'],
    read_properties: ['has'],
    interceptors: {
        add: (notify_read_methods, value, property, ...params) => {
            if (value.has(params[0])) {
                return false;
            }
            notify_read_methods(['has'], params[0]);
            return true;
        },
        clear: (notify_read_methods, value, property, ...params) => {
            if (value.size == 0) {
                return false;
            }
            notify_read_methods(['has'], NOTIFY_WITH_ALL_REGISTERED_PARAMS);
            return true;
        },
        delete: (notify_read_methods, value, property, ...params) => {
            if (!value.has(params[0])) {
                return false;
            }
            notify_read_methods(['has'], params[0]);
            return true;
        }
    }
});

This has three main parts:

  1. write_properties: Write properties that could cause reactivity based on inputs, other internal stuff, or always.
  2. read_properties: Read properties that shouldn’t be reactive with each change but based on some conditions. Anything in this list should be notified of changes manually by calling notify_read_methods.
  3. interceptors: A handler called before a property in write_properties is invoked.

The magic happens in the interceptors. They give us the current value (before the change happens), the parameters that the write property is invoked with (if any), and a method called notify_read_methods that can be invoked for any property listed in read_properties.

Breaking Down the clear Interceptor:

  1. In the clear method, we don't want to cause reactivity if the set is already empty, so we return early.
  2. If it's not empty, notify all has read methods that are already registered. For instance, if we never called set.has(1000), why bother?
  3. You might wonder why not notify size. As mentioned earlier, anything not listed in read_properties will be notified automatically based on any change. If the interceptor returns false, we won't increment an internal signal called version (just like the current implementation). But if it returns true or we call notify_read_methods, it tells the utility to increment the version signal so other properties not listed in read_properties can get notified.

Breaking Down the add Interceptor:

  1. If the internal value (the set) already has the parameter it's called with (let’s call it x), return early.
  2. If it doesn't exist in the set, notify all has functions called with x. This will also increment the internal version signal, which will notify size, values, etc.
  3. If we didn't call notify_read_methods and just returned true, that would also increment the version signal.

With this setup, it's easy to be as fine-grained as possible without data duplication and make any class reactive in a generic way without worrying about data management. All you care about is who notifies who and what should be reactive.

Relates to #11200, #11385, #11233, #11235 and #11287.
Fixes #11727, the log problem part of (#11233) and #11680

Svelte 5 rewrite

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

Copy link

changeset-bot bot commented May 7, 2024

⚠️ No Changeset found

Latest commit: b2e7e30

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

@FoHoOV FoHoOV changed the title chore: a new take on reactivity package fix: a new take on reactivity package May 7, 2024
@FoHoOV
Copy link
Author

FoHoOV commented May 8, 2024

This is still in draft mode because in this code:

<script>
	import {Set} from "svelte/reactivity";	

	const data = $state([1,2,3]);
	const reactiveSet = new Set(data);
	
	$effect(() => {
		console.log("has 1", reactiveSet.has(1));
	});

	$effect(() => {
		console.log("has 2", reactiveSet.has(2));
	});

	$effect(() => {
		console.log("has 3", reactiveSet.has(3));
	});

	$effect(() => {
		console.log("has 5", reactiveSet.has(5));
	});
</script>

<button onclick={()=>{
	reactiveSet.delete(2);
}}>
	delete 2
</button>


<button onclick={()=>{
	reactiveSet.add(2);
}}>
	add 2
</button>


<button onclick={()=>{
	reactiveSet.clear();
}}>
	clear
</button>

when we delete 2 from the set, all effects run where we call reactiveSet.has(SOMETHING). Keeping in mind that delete on a value that doesn't exist will not rerun the effects or deleting a value that exists twice doesn't rerun the effect for the second time. Following the current behaviour:

<script>
	import {Set} from "svelte/reactivity";	

	const data = $state([1,2]);
	const reactiveSet = new Set(data);
	
	$effect(() => {
		console.log("has 1", reactiveSet.has(1));
	});

	$effect(() => {
		console.log("has 2", reactiveSet.has(2));
	});
</script>

<button onclick={()=>{
	reactiveSet.delete(2);
}}>
	delete 2
</button>

after clicking on "delete 2" only reactiveSet.has(2) should get called (or things simillar to this case). Everything else works.

@7nik
Copy link

7nik commented May 8, 2024

As you saw, the general problem with this approach is that the object isn't fine-grained. You want

$effect(() => {
  console.log(reactiveMap.get(42));
});

run only when a value with the key 42 is added or set but not any other.
You can turn each reading method into a derived so that the effect won't rerun, but then, in a set/map with 10k items, each change will trigger 10k derives, which isn't performant. So, the most performant way is again to have a signal for each item, but you pay with memory for it.

There are also some debates about whether the stored item should be reactified.

@Azarattum
Copy link
Contributor

Azarattum commented May 9, 2024

I feel like this is quite nice generic solution for making arbitrary things reactive easily (e.g. objects from 3rd party libraries that you have no control over). However as @7nik pointed out, this isn't fine-grained. So, I think that basic things like Set/Map should be implemented manually, to make them as fine-grained as possible.

@FoHoOV
Copy link
Author

FoHoOV commented May 9, 2024

@7nik @Azarattum, I completely agree with you. I've marked this as a draft while I try to comeup with a solution (hopefully) that addresses this issue. However, as noted in issue #11515, the current implementation still faces a similar challenge from a different angle. For example, if a set contains a single item and we execute set.has(X) 1000 times within an effect, where X is not an element of the set, with each modification it results in at most 1000 has calls that haven't changed. Unless each of those have their own signals which results in 1000 signals that might be an unnecessary overhead. I'm honestly not sure how to resolve this effectively. Any ideas?

<script>
	import {Set} from "svelte/reactivity";	

	const data = [1];
	const reactiveSet = new Set(data);
	
	$effect(() => {
		console.log("has 1", reactiveSet.has(1));
	});

	$effect(() => {
		console.log("has 2", reactiveSet.has(2));
	        console.log("has 3", reactiveSet.has(3));
                // and so one for another 100000 elements that dont exist in the set and might never do!
	});
</script>

basically each read like method might create many unnecessary signals.

@Azarattum
Copy link
Contributor

Azarattum commented May 9, 2024

Well, to react to a 1000 items changing finely, we would need a 1000 signals. I think in this case this is what the user has requested explicitly, so it feels like an expected behavior. I think having a lot of signals that trigger less is better than a few that trigger often. This is what the fine-grainness is all about. Though I think that it would still be more common to have less signals than items in a set. So, lazy signal initialization shouldn't be an issue.

@7nik
Copy link

7nik commented May 9, 2024

I thought about storing in Map super items of shape { signal, value }, but this approach won't work for Set.
So, I lean toward storing values in the super and signals in a private map. Plus, it allows not to create signals for keys that were never read.

Regarding reactivity for non-existing keys, just creating and storing signals for them can cause a memory leak when somebody uses huge objects as keys or adds and removes thousands of unique keys. Temporary returning derived (kind of $deirvied( (this.#version, super.has(key)) )) may cause bad performance when somebody watches for thousands of non-existing keys simultaneously (though how likely it is?). Another alternative is StartStopNotifier, which removes the signal when nobody listens to it anymore.

@Azarattum
Copy link
Contributor

Azarattum commented May 10, 2024

@7nik, #11287 does kind of implement StartStopNotifer pattern. Though the implementation itself still isn't very elegant...

@03juan
Copy link

03juan commented May 10, 2024

So, I lean toward storing values in the super and signals in a private map. Plus, it allows not to create signals for keys that were never read.

Regarding reactivity for non-existing keys, just creating and storing signals for them can cause a memory leak when somebody uses huge objects as keys or adds and removes thousands of unique keys.

@7nik wouldn't this be a good candidate for a WeakMap for storing the signals, to not hold strongly to the user's potentially huge Map key or Set value?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap#emulating_private_members

@FoHoOV FoHoOV marked this pull request as ready for review May 10, 2024 18:34
@FoHoOV
Copy link
Author

FoHoOV commented May 10, 2024

fixed some issues thanks to the comments above, also fixes #11515

@FoHoOV FoHoOV changed the title fix: a new take on reactivity package fix: generic utility for making built-in classes reactive May 12, 2024
@FoHoOV FoHoOV changed the title fix: generic utility for making built-in classes reactive fix: generic utility for making any classes reactive May 16, 2024
@FoHoOV FoHoOV changed the title fix: generic utility for making any classes reactive fix: generic utility for making any classe reactive May 16, 2024
@FoHoOV FoHoOV changed the title fix: generic utility for making any classe reactive fix: generic utility for making any class reactive May 16, 2024
@FoHoOV FoHoOV changed the title fix: generic utility for making any class reactive feat: generic utility for making any class reactive May 16, 2024
@FoHoOV FoHoOV force-pushed the improve-reactivity-package branch 2 times, most recently from 31800ee to 1457398 Compare May 17, 2024 07:07
@FoHoOV FoHoOV marked this pull request as draft May 18, 2024 12:27
@FoHoOV
Copy link
Author

FoHoOV commented May 18, 2024

I came up with better idea to make it more accurate. Because currently it makes signals aware that a change "is going to happen", the correct implementation would be "a change has happened".
Edit: Fixed now

@trueadm
Copy link
Contributor

trueadm commented May 20, 2024

I've not had time to look into this deeply yet, will get around to it this week! Also, the lint CI workflow is failing :)

@FoHoOV
Copy link
Author

FoHoOV commented May 20, 2024

I've not had time to look into this deeply yet, will get around to it this week! Also, the lint CI workflow is failing :)

Reallyyyy looking forward for your feedback >_<
also the linter issue is so weird, because that should actually work. used another way to fix it though

@FoHoOV FoHoOV force-pushed the improve-reactivity-package branch from bdc860f to 705e9d5 Compare May 22, 2024 10:40
@FoHoOV
Copy link
Author

FoHoOV commented May 22, 2024

@trueadm today I actually had time and changed other reactivity package classes to use the this utility as well. marking this as draft again till I complete those

@FoHoOV FoHoOV marked this pull request as draft May 22, 2024 10:56
@FoHoOV FoHoOV marked this pull request as ready for review May 22, 2024 11:47
@FoHoOV FoHoOV force-pushed the improve-reactivity-package branch from d58d6ce to aaf329a Compare May 22, 2024 12:49
@FoHoOV
Copy link
Author

FoHoOV commented May 23, 2024

I updated the PR description to include a detailed explanation of what this change does.

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

Successfully merging this pull request may close these issues.

svelte5: reactivity package classes are not fine-grained
5 participants