-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix: improve reactive Map and Set implementations #11827
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
Conversation
🦋 Changeset detectedLatest commit: ac921d8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@fznhq There's nothing we can do about Firefox unfortunately. It doesn't like having a |
If there is nothing svelte can do about it, just leave a note in the documentation about this behavior. Thanks @trueadm |
@@ -28,7 +27,8 @@ export class ReactiveMap extends Map { | |||
var sources = this.#sources; | |||
|
|||
for (var [key, v] of value) { | |||
sources.set(key, source(v)); | |||
sources.set(key, source(Symbol())); |
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.
Shouldn't we check if the key
is an object here as well?
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.
By the way, I'm not sure if we need to fill the sources
Map here, since we now generate signals in has
and get
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.
Good point, fixed.
|
||
/** | ||
* @template K | ||
* @template V | ||
* @extends {Map<K, V>} | ||
*/ | ||
export class ReactiveMap extends Map { | ||
/** @type {Map<K, import('#client').Source<V>>} */ | ||
/** @type {Map<K, import('#client').Source<symbol>>} */ | ||
#sources = new Map(); |
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 we could use a WeakMap here to allow for object keys?
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.
Nah WeakMaps don't give us anything here really. It means we'd have to duplicate data as we need to store the value on the super
and in the WeakMap, which means the WeakMap never does anything as the value is strongly held. Also WeakMaps are very slow.
// If we're working with a non-primitive then we can generate a signal for it | ||
if (typeof key !== 'object' || key === null) { | ||
s = source(UNINITIALIZED); | ||
sources.set(key, s); |
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.
This still has the same problem as #11504 and #11287. We need a way to remove these signals when we are no longer listening to them. Otherwise it would create a memory leak (see #11504 (comment))
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.
Fixed. Unfortunately, this means that we can't retain primitives either. So they won't be fine-grain, as there's no way to do so without leaking.
There are still two major problems with this:
|
Fixed these issues. Unfortunately, checks for values that aren't added will not be fine-grain till they are added. There's nothing we can do about this without retaining too much memory. If you do a loop and does a |
Closes #11727.
This PR aims to tackle issues around our reactive Map/Set implementations. Notably: