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.

Sign up for GitHub

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

Jump to bottom

breaking: finer lazy reactivity for set #11287

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

breaking: finer lazy reactivity for set #11287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2063ca
breaking: finer lazy reactivity for set
Azarattum Apr 22, 2024
69bae17
fix: defer call to has till needed
Azarattum Apr 23, 2024
3775935
chore: regenerate and improve set typings
Azarattum Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
68 changes: 37 additions & 31 deletions packages/svelte/src/reactivity/set.js
View file
Open in desktop
@@ -1,5 +1,6 @@
import { DEV } from 'esm-env';
import { source, set } from '../internal/client/reactivity/sources.js';
import { effect } from '../internal/client/reactivity/effects.js';
import { get } from '../internal/client/runtime.js';
import { map } from './utils.js';

Expand All @@ -10,10 +11,11 @@ var inited = false;

/**
* @template T
* @extends {Set<T>}
*/
export class ReactiveSet extends Set {
/** @type {Map<T, import('#client').Source<boolean>>} */
#sources = new Map();
#tracked = new Map();
#version = source(0);
#size = source(0);

Expand All @@ -27,13 +29,11 @@ export class ReactiveSet extends Set {
if (DEV) new Set(value);

if (value) {
var sources = this.#sources;

for (var element of value) {
sources.set(element, source(true));
super.add(element);
}

this.#size.v = sources.size;
this.#size.v = super.size;
}

if (!inited) this.#init();
Expand Down Expand Up @@ -73,65 +73,71 @@ export class ReactiveSet extends Set {

/** @param {T} value */
has(value) {
var s = this.#sources.get(value);
var s = this.#tracked.get(value);

if (s === undefined) {
// We should always track the version in case
// the Set ever gets this value in the future.
get(this.#version);

return false;
s = source(super.has(value));
this.#tracked.set(value, s);
}

effect(() => () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

return get(s);
}

/** @param {T} value */
add(value) {
var sources = this.#sources;

if (!sources.has(value)) {
sources.set(value, source(true));
set(this.#size, sources.size);
if (!super.has(value)) {
super.add(value);
set(this.#size, super.size);
this.#increment_version();

var s = this.#tracked.get(value);
if (s !== undefined) {
set(s, true);
}
}

return this;
}

/** @param {T} value */
delete(value) {
var sources = this.#sources;
var s = sources.get(value);

if (s !== undefined) {
var removed = sources.delete(value);
set(this.#size, sources.size);
set(s, false);
var removed = super.delete(value);
if (removed) {
set(this.#size, super.size);
this.#increment_version();
return removed;

var s = this.#tracked.get(value);
if (s !== undefined) {
set(s, false);
}
}

return false;
return removed;
}

clear() {
var sources = this.#sources;

if (sources.size !== 0) {
if (super.size !== 0) {
set(this.#size, 0);
for (var s of sources.values()) {
for (var s of this.#tracked.values()) {
set(s, false);
}
this.#increment_version();
}

sources.clear();
super.clear();
}

keys() {
get(this.#version);
return map(this.#sources.keys(), (key) => key, 'Set Iterator');
return map(super.keys(), (key) => key, 'Set Iterator');
}

values() {
Expand Down
6 changes: 5 additions & 1 deletion packages/svelte/src/reactivity/set.test.ts
View file
Open in desktop
Expand Up @@ -30,7 +30,7 @@ test('set.values()', () => {
set.clear();
});

assert.deepEqual(log, [5, true, [1, 2, 3, 4, 5], 4, false, [1, 2, 4, 5], 0, false, []]);
assert.deepEqual(log, [5, true, [1, 2, 3, 4, 5], 4, false, [1, 2, 4, 5], 0, []]);

cleanup();
});
Expand Down Expand Up @@ -58,6 +58,10 @@ test('set.has(...)', () => {
set.delete(2);
});

flushSync(() => {
set.add(4);
});

flushSync(() => {
set.add(2);
});
Expand Down
10 changes: 1 addition & 9 deletions packages/svelte/types/index.d.ts
View file
Open in desktop
Expand Up @@ -1984,19 +1984,11 @@ declare module 'svelte/reactivity' {
constructor(...values: any[]);
#private;
}
class ReactiveSet<T> extends Set<any> {
class ReactiveSet<T> extends Set<T> {

constructor(value?: Iterable<T> | null | undefined);

has(value: T): boolean;

add(value: T): this;

delete(value: T): boolean;
keys(): IterableIterator<T>;
values(): IterableIterator<T>;
entries(): IterableIterator<[T, T]>;
[Symbol.iterator](): IterableIterator<T>;
#private;
}
class ReactiveMap<K, V> extends Map<any, any> {
Expand Down