-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Use local user css cache for apply #7524
Merged
Merged
Conversation
This file contains 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
thecrypticace
force-pushed
the
fix/context-invalidation-for-apply
branch
2 times, most recently
from
February 24, 2022 03:13
895e521
to
6c4aa56
Compare
We were storing user CSS in the context so we could use it with apply. The problem is that this CSS does not get updated on save unless it has a tailwind directive in it resulting in stale apply caches. This could result in either stale generation or errors about missing classes.
thecrypticace
force-pushed
the
fix/context-invalidation-for-apply
branch
2 times, most recently
from
February 24, 2022 03:25
549f40c
to
8fccb99
Compare
The previous method was very inefficient as PostCSS was recursively cloning the entire tree. We only care about cloning the node itself as we’re replacing its children. This is about 4x faster on large CSS blobs.
RobinMalfait
requested changes
Feb 25, 2022
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.
Functionally it seems to work great, nice!
I do have some questions / nitpicks.
RobinMalfait
approved these changes
Feb 25, 2022
@thecrypticace thanks for working on that! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Background
The PR #6580 caused any file using apply to create a new context. This meant that any file that used apply would also carry along the entire content dependency tree. Updating any file covered by
content
would cause these files to recompile. The original cause of the problem that #6580 was fixing turned out to be a cache invalidation issue. Since user CSS was added to the context things could end up incorrectly shared across files when it comes to apply. However, this would only happen once and future updates would not invalidate the cache. This would manifest as an error about the applied classes being missing in some cases.Why did it seem to fix the original problem?
By creating a new context for any file containing apply it caused the context to get re-created every time that file was saved. Because of this the CSS was re-scanned and added to the internal user CSS cache that we had on the context.
What are we going to do instead
This is still a work in progress. Some of the code here needs to be cleaned up first.
Fixes #7480