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 71 commits into
base: main
Choose a base branch
from

Conversation

FoHoOV
Copy link
Contributor

@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: d3fd750

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
Contributor 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
Contributor 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
Contributor 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
Contributor 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
Contributor 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 1a7213c to d3fd750 Compare June 1, 2024 17:04
@FoHoOV
Copy link
Contributor Author

FoHoOV commented Jun 1, 2024

@trueadm Yes from memory perspective this implementation could actually retain more memory as it goes on. But maybe doing these solutions can help?

... based on that and #11287 (comment), we can do the following:

  1. Avoid creating signals if we are not inside an effect or a derived state. I'm not that familiar with Svelte's internals, so what's the correct approach for this?
  2. Remove unused param-to-signal key-value pairs.

If I actually opt into using a reactive set/map/url/date/..., I would expect it to be reactive just like everything else in svelte (this package is comming from svelte as well). So I think not being reactive when keys are objects, loosing reactivity when we clear a set/map or other things mentioned in #11727 might not be an expected behavior. The challenge is we really need to know which read function is called with what paremeter(s) (or getters with no parameters), so it could be reactive based on exactly that (or somebody else affecting it). But we might be able to group some read-methods to use the same set of signals. For instance we can group has/get into one set of signals. What I mean is do you think its worth trying to optimize this or the memory cost just doesn't worth it?

My highlights on this:

  1. being able to create actually fine-grained reactive classes from existing classes (without worring about data-management)
  2. being able to fix/optimize stuff in a signle place (utils.js)
  3. easy to use API which makes it easier to make other classes reactive in future

@trueadm
Copy link
Contributor

trueadm commented Jun 1, 2024

It’s more important to be efficient with memory than have greater fine grained reactivity. We can’t create signals only in reactive contexts either, we used to do that for objects and arrays but it was causing bugs so we stopped doing it. The truth is using a dynamic wrapper is always going to be more expensive than doing it statically on the prototype as the JIT can optimize it better. Performance of runtime is even more critical than the theoretical benefits of fine grain performance - the best pattern is actually somewhere in the middle. That’s why we share a single effect for as much the template as possible rather than have many small fine grain ones.

@FoHoOV
Copy link
Contributor Author

FoHoOV commented Jun 1, 2024

@trueadm Thanks for the review!! My whole mindset was around making stuff as fine-grained as possible and not worrying about memory at that moment then try to make it performant in later step xD I was thinking in THE OPPOSITE direction🫡dayumn

  • Many thanks for giving me your time >_<

@trueadm
Copy link
Contributor

trueadm commented Jun 1, 2024

@FoHoOV Signals aren't free at the end of the day, we have to do a lot of bookkeeping around them to manage their relationships. If we're doing this for things that might leak, then it's surely not worth it. Thank you for digging into this, you've been awesome :)

We still need to fix Date though right? How do you feel about tackling that without the usage of a wrapper?

@FoHoOV
Copy link
Contributor Author

FoHoOV commented Jun 1, 2024

@TrueDM Thanks! Sure, if by the time I start nobody has done it yet, I'll try to do it for the url, date and searchparams.

@7nik
Copy link

7nik commented Jun 1, 2024

Does Date need to be fixed? I see Date as a wrapped timestamp with a bunch of methods to format/modify it. So, it doesn't seem efficient to create signals (even if on-demand only) for all read methods. And it is easy to stop over-reactivity with $derived on userland.

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