Merged
Conversation
supports conditions
RyanZim
reviewed
Dec 26, 2023
Collaborator
RyanZim
left a comment
There was a problem hiding this comment.
Just to clarify, base64 encoding is only used for external URL imports, right?
A few questions below as well; otherwise, looks good.
Collaborator
Author
|
Thank you for the review 🙇
Yes. |
RyanZim
approved these changes
Dec 28, 2023
Collaborator
|
Thanks a lot! Ready to publish as v16? |
Collaborator
|
I think I'll wait until after the New Year to publish, just in case something goes wrong. |
Collaborator
|
025f27a 🎉 |
Collaborator
Author
|
Awesome! thank you so much for all the careful reviews 🙇 |
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
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.
supersedes : #532
fixes : #529
The core issue is how multiple statements are combined.
The previous implementation tried to squash media queries and layer conditions.
layer(foo) screenlayer(bar) (min-width: 1000px)becomes :
In this example the
foolayer is only defined when the media isscreenand has a width of1000px. While it should actually be defined even when the media isscreenand smaller.screennot printbecomes :
In this example the media query becomes invalid because
andcan not be used to combinescreenandnot printThe implementation for this feature was also fairly complex.
Adding
supportsinto this would have made the implementation very unwieldy and it would have increased the chance that users experience bugs.The correct way to combine is actually much simpler.
layer(foo) screenlayer(bar) (min-width: 1000px)becomes :
screennot printbecomes :
By using nested statements we ensure that the order of declaration is respected and that no invalid statements are produced.
At this point it also becomes trivial to add support for
supportsconditions.This method does break one aspect.
@importstatements can not be nested syntactically in CSS.They must appear first at the root of the AST.
valid :
invalid :
To restore support for this we can use nested stylesheets.
@importstatement urls can be base64 encoded data urls.This doesn't actually have to be base64 encoded, but encoding makes it less prone to errors.
Without encoding it looks like this :
@imports url('http://something-external.com') not print;screenmedia conditionThis technique was first thought of by the maintainer of esbuild while fixing issues brought to light by https://github.com/romainmenke/css-import-tests
This implementation change fixes a lot of subtle bugs and as far as I can tell the output is almost always smaller.
It also eliminates some complexity for users.
They no longer need to define things like
nameLayerfor anonymous layers to work correctly. The issue for which we introduced that option no longer exists in this implementation.So I think this is a pure win :)