Initial implementation of cache cleanup #22510
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
josephsavona
changed the title
josephsavona
commented
Oct 5, 2021
josephsavona
commented
Oct 5, 2021
acdlite
reviewed
Oct 6, 2021
acdlite
reviewed
Oct 6, 2021
josephsavona
commented
Oct 7, 2021
acdlite
reviewed
Oct 8, 2021
acdlite
reviewed
Oct 8, 2021
acdlite
reviewed
Oct 8, 2021
josephsavona
commented
Oct 8, 2021
josephsavona
commented
Oct 8, 2021
josephsavona
commented
Oct 8, 2021
| ) { | ||
| if (!rootDoesHavePassiveEffects) { | ||
| rootDoesHavePassiveEffects = true; | ||
| pendingPassiveEffectsRemainingLanes = remainingLanes; |
Member
Author
There was a problem hiding this comment.
we need to store this to know which lanes were remaining when flushPassiveEffects() runs asynchronously
josephsavona
commented
Oct 8, 2021
Member
Author
|
Okay @acdlite this is ready for another review pass! |
josephsavona
commented
Oct 12, 2021
packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js
Outdated
Show resolved
Hide resolved
josephsavona
changed the title
acdlite
reviewed
Oct 19, 2021
acdlite
reviewed
Oct 19, 2021
acdlite
reviewed
Oct 19, 2021
acdlite
reviewed
Oct 19, 2021
josephsavona
commented
Oct 20, 2021
|
Comparing: bfb4022...dcf04a4 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
josephsavona
force-pushed
the
cache-cleanup-2
branch
from
db7944f to
4ba534c
Compare
acdlite
approved these changes
Oct 21, 2021
Collaborator
acdlite
left a comment
acdlite
left a comment
There was a problem hiding this comment.
🎉🎉🎉 Yay! Thanks so much for pushing this feature through! I think the rest we can address in follow up PRs.
Looks like you'll need to pull upstream changes and resolve them one more time before merging.
Very exciting!
josephsavona
force-pushed
the
cache-cleanup-2
branch
from
f2a9fc7 to
6fae379
Compare
This comment has been minimized.
This comment has been minimized.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is an initial, partial implementation of a cleanup mechanism for the experimental Cache API. The idea is that consumers of the Cache API can register to be informed when a given Cache instance is no longer needed so that they can perform associated cleanup tasks to free resources stored in the cache. A canonical example would be cancelling pending network requests.
An overview of the high-level changes:
Cachetype from a Map of cache instances to be an object with the original Map of instances, a reference count (to count roughly "active references" to the cache instances - more below), and an AbortController.unstable_getCacheSignal(): AbortSignal, which is callable during render. It returns an AbortSignal tied to the lifetime of the cache - developers can listen for the 'abort' event on the signal, which React now triggers when a given cache instance is no longer referenced.AbortSignalis a web standard that is supported by other platform APIs; for example a signal can be passed tofetch()to trigger cancellation of an HTTP request.Cases handled:
For follow-ups:
How did you test this change?
yarn test --all- a few tests are still failing and i'll need some guidance on how to resolve those (i'll post more details later).