-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
DocumentTransform
: use a WeakCache
#11388
Conversation
🦋 Changeset detectedLatest commit: 263ae13 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 |
size-limit report 📦
|
@@ -595,7 +595,7 @@ export class DocumentTransform { | |||
concat(otherTransform: DocumentTransform): DocumentTransform; | |||
// (undocumented) | |||
getStableCacheEntry(document: DocumentNode): { | |||
key: DocumentTransformCacheKey; | |||
key?: undefined; |
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 is a kinda-minor breaking change here - if we kept this key
in memory here, we would again risk strongly referencing the original DocumentNode
.
It might be fine to still keep it in here, and if we decide on that, I'll experiment if this will actually hinder it from cache-collection, but on the other hand we also just don't need this property.
@jerelmiller what's your feeling on this change?
@@ -134,7 +144,14 @@ export class DocumentTransform { | |||
Array.isArray(cacheKeys), | |||
"`getCacheKey` must return an array or undefined" | |||
); | |||
return this.stableCacheKeys.lookupArray(cacheKeys); | |||
const key = this.stableCacheKeys.lookupArray(cacheKeys); |
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.
Essentially, this PR decouples the Trie
and the data - the Trie
only stores the cache keys (which are empty objects, not the array used to access the Trie
), while the data is stored in the WeakCache
.
This enables us to have a max
size, which is impossible for a Trie
, but possible for a WeakCache
.
This is option 1 of 2 - I'll open a PR going a bit further and replacing a lot of the logic with
optimism
in a bit. Will be linked here.Checklist: