-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix: use state
instead of source
in reactive classes
#16239
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
Changes from 7 commits
de8053e
2e69ed4
fb627c0
2ea891e
5670a21
f261686
e3072c9
3ab41d6
69ebe29
b208f71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,10 @@ | ||
/** @import { Source } from '#client' */ | ||
/** @import { Reaction, Source } from '#client' */ | ||
import { DEV } from 'esm-env'; | ||
import { source, set, state } from '../internal/client/reactivity/sources.js'; | ||
import { label, tag } from '../internal/client/dev/tracing.js'; | ||
import { get } from '../internal/client/runtime.js'; | ||
import { active_reaction, get } from '../internal/client/runtime.js'; | ||
import { increment } from './utils.js'; | ||
import { teardown } from '../internal/client/reactivity/effects.js'; | ||
|
||
var read_methods = ['forEach', 'isDisjointFrom', 'isSubsetOf', 'isSupersetOf']; | ||
var set_like_methods = ['difference', 'intersection', 'symmetricDifference', 'union']; | ||
|
@@ -51,12 +52,22 @@ export class SvelteSet extends Set { | |
#version = state(0); | ||
#size = state(0); | ||
|
||
/**@type {WeakRef<Reaction> | null}*/ | ||
#initial_reaction = null; | ||
|
||
/** | ||
* @param {Iterable<T> | null | undefined} [value] | ||
*/ | ||
constructor(value) { | ||
super(); | ||
|
||
if (active_reaction !== null) { | ||
// we use a WeakRef (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakRef) | ||
// so that if this Map is somehow stored outside of the active reaction, | ||
// it will not prevent the reaction from being garbage collected. | ||
this.#initial_reaction = new WeakRef(active_reaction); | ||
} | ||
|
||
if (DEV) { | ||
// If the value is invalid then the native exception will fire here | ||
value = new Set(value); | ||
|
@@ -75,6 +86,22 @@ export class SvelteSet extends Set { | |
if (!inited) this.#init(); | ||
} | ||
|
||
/** | ||
* If the active_reaction is the same as the reaction that created this SvelteMap, | ||
* we use state so that it will not be a dependency of the reaction. Otherwise we | ||
* use source so it will be. | ||
* | ||
* @template T | ||
* @param {T} value | ||
* @returns {Source<T>} | ||
*/ | ||
#source(value) { | ||
if (this.#initial_reaction !== null && this.#initial_reaction.deref() === active_reaction) { | ||
return state(value); | ||
} | ||
return source(value); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be better to DRY and move this (and the one in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe but then it would either be a class that they extend or it would need to have the reaction passed (or called with |
||
// We init as part of the first instance so that we can treeshake this class | ||
#init() { | ||
inited = true; | ||
|
@@ -116,7 +143,7 @@ export class SvelteSet extends Set { | |
return false; | ||
} | ||
|
||
s = source(true); | ||
s = this.#source(true); | ||
|
||
if (DEV) { | ||
tag(s, `SvelteSet has(${label(value)})`); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,101 @@ | ||
<script> | ||
import { SvelteMap } from 'svelte/reactivity'; | ||
|
||
let visibleExternal = $state(false); | ||
let external = new SvelteMap(); | ||
const throws = $derived.by(() => { | ||
external.set(1, 1); | ||
return external; | ||
let outside_basic = $state(false); | ||
let outside_basic_map = new SvelteMap(); | ||
const throw_basic = $derived.by(() => { | ||
outside_basic_map.set(1, 1); | ||
return outside_basic_map; | ||
}); | ||
|
||
let visibleInternal = $state(false); | ||
const works = $derived.by(() => { | ||
let internal = new SvelteMap(); | ||
internal.set(1, 1); | ||
return internal; | ||
let inside_basic = $state(false); | ||
const works_basic = $derived.by(() => { | ||
let inside = new SvelteMap(); | ||
inside.set(1, 1); | ||
return inside; | ||
}); | ||
|
||
let outside_has = $state(false); | ||
let outside_has_map = new SvelteMap([[1, 1]]); | ||
const throw_has = $derived.by(() => { | ||
outside_has_map.has(1); | ||
outside_has_map.set(1, 2); | ||
return outside_has_map; | ||
}); | ||
|
||
let inside_has = $state(false); | ||
const works_has = $derived.by(() => { | ||
let inside = new SvelteMap([[1, 1]]); | ||
inside.has(1); | ||
inside.set(1, 1); | ||
return inside; | ||
}); | ||
|
||
let outside_get = $state(false); | ||
let outside_get_map = new SvelteMap([[1, 1]]); | ||
const throw_get = $derived.by(() => { | ||
outside_get_map.get(1); | ||
outside_get_map.set(1, 2); | ||
return outside_get_map; | ||
}); | ||
|
||
let inside_get = $state(false); | ||
const works_get = $derived.by(() => { | ||
let inside = new SvelteMap([[1, 1]]); | ||
inside.get(1); | ||
inside.set(1, 1); | ||
return inside; | ||
}); | ||
|
||
let outside_values = $state(false); | ||
let outside_values_map = new SvelteMap([[1, 1]]); | ||
const throw_values = $derived.by(() => { | ||
outside_values_map.values(1); | ||
outside_values_map.set(1, 2); | ||
return outside_values_map; | ||
}); | ||
|
||
let inside_values = $state(false); | ||
const works_values = $derived.by(() => { | ||
let inside = new SvelteMap([[1, 1]]); | ||
inside.values(); | ||
inside.set(1, 1); | ||
return inside; | ||
}); | ||
</script> | ||
|
||
<button onclick={() => (visibleExternal = true)}>external</button> | ||
{#if visibleExternal} | ||
{throws} | ||
<button onclick={() => (outside_basic = true)}>external</button> | ||
{#if outside_basic} | ||
{throw_basic} | ||
{/if} | ||
<button onclick={() => (inside_basic = true)}>internal</button> | ||
{#if inside_basic} | ||
{works_basic} | ||
{/if} | ||
|
||
<button onclick={() => (outside_has = true)}>external</button> | ||
{#if outside_has} | ||
{throw_has} | ||
{/if} | ||
<button onclick={() => (visibleInternal = true)}>internal</button> | ||
{#if visibleInternal} | ||
{works} | ||
<button onclick={() => (inside_has = true)}>internal</button> | ||
{#if inside_has} | ||
{works_has} | ||
{/if} | ||
|
||
<button onclick={() => (outside_get = true)}>external</button> | ||
{#if outside_get} | ||
{throw_get} | ||
{/if} | ||
<button onclick={() => (inside_get = true)}>internal</button> | ||
{#if inside_get} | ||
{works_get} | ||
{/if} | ||
|
||
<button onclick={() => (outside_values = true)}>external</button> | ||
{#if outside_values} | ||
{throw_values} | ||
{/if} | ||
<button onclick={() => (inside_values = true)}>internal</button> | ||
{#if inside_values} | ||
{works_values} | ||
{/if} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if there are cases when map still lives, while the reaction could have been GCed and this would prevent it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was wondering about that too...technically it shouldn't be the case because either there's no active reaction (it's outside of a derived/effect) or it's inside it but this means that when the derived/effect is no longer used the function should be GCd and everything inside it too...but I need to do a better check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this can become
initial_reaction = WeakSet(active_reaction)
and we can check withthis.initial_reaction.has(active_reaction)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for the sake of history, I've also discovered there is a WeakRef struct that could be alternative here. In case of any future issues might be a possible alternative implementation