Skip to content
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

implement supports conditions #548

Conversation

romainmenke
Copy link
Collaborator

@romainmenke romainmenke commented Dec 9, 2023

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) screen
  • layer(bar) (min-width: 1000px)

becomes :

@media screen and (min-width: 1000px) {
  @layer foo.bar {}
}

In this example the foo layer is only defined when the media is screen and has a width of 1000px. While it should actually be defined even when the media is screen and smaller.

  • screen
  • not print

becomes :

@media screen and not print {}

In this example the media query becomes invalid because and can not be used to combine screen and not print


The implementation for this feature was also fairly complex.

Adding supports into 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) screen
  • layer(bar) (min-width: 1000px)

becomes :

@media screen {
  @layer foo {
    @media (min-width: 1000px) {
      @layer foo.bar {}
    }
  }
}
  • screen
  • not print

becomes :

@media screen {
  @media not print {}
}

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 supports conditions.


This method does break one aspect.

@import statements can not be nested syntactically in CSS.
They must appear first at the root of the AST.

valid :

@imports "http://something-external.com" screen;

invalid :

@media screen {
  @imports "http://something-external.com";
}

To restore support for this we can use nested stylesheets.
@import statement 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 :

@import url("data:text/css,@imports url('http://something-external.com') not print;") screen;
  • import this : @imports url('http://something-external.com') not print;
  • apply a screen media condition

This 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 nameLayer for 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 :)

@romainmenke romainmenke changed the title implement supports conditions implement supports conditions Dec 9, 2023
Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, base64 encoding is only used for external URL imports, right?

A few questions below as well; otherwise, looks good.

test/layer.js Outdated Show resolved Hide resolved
lib/process-content.js Outdated Show resolved Hide resolved
@romainmenke
Copy link
Collaborator Author

Thank you for the review 🙇

Just to clarify, base64 encoding is only used for external URL imports, right?

Yes.
Only when an import can not be inlined and conditions or cascade layers need to be applied.

@RyanZim RyanZim merged commit c262a30 into master Dec 28, 2023
3 checks passed
@RyanZim RyanZim deleted the implement-supports-conditions--exuberant-newfoundland-c56d10123c branch December 28, 2023 14:38
@RyanZim
Copy link
Collaborator

RyanZim commented Dec 28, 2023

Thanks a lot! Ready to publish as v16?

@RyanZim
Copy link
Collaborator

RyanZim commented Dec 29, 2023

I think I'll wait until after the New Year to publish, just in case something goes wrong.

@RyanZim
Copy link
Collaborator

RyanZim commented Jan 3, 2024

025f27a 🎉

@romainmenke
Copy link
Collaborator Author

Awesome! thank you so much for all the careful reviews 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supports at rule not working
2 participants