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

layers support #483

Conversation

romainmenke
Copy link
Collaborator

@romainmenke romainmenke commented Feb 19, 2022

With @layer support soon in all browsers stylesheet authors will also start using these in @import.

spec : https://drafts.csswg.org/css-cascade-5/#at-import

new syntax :

@import [ <url> | <string> ]
        [ layer |  layer(<layer-name>) ]?
        <import-condition> ;

Because I am not familiar with the codebase I first wanted to checkin to make sure I am on the right track.

Status :

  • detects and parses layer
  • failing simple test
  • transform
  • increase test coverage

The intention is to have this transform :

@import "foo.css" layer;

/* becomes */

@layer {
  foo {}
}

I could not easily find the best place to intervene and create a new atRule for the layers. Is this something you can help out with @ai?

@RyanZim
Copy link
Collaborator

RyanZim commented Feb 21, 2022

@romainmenke See how we do it for media imports here:

postcss-import/index.js

Lines 79 to 83 in e2d2890

const mediaNode = atRule({
name: "media",
params: stmt.media.join(", "),
source: parent.source,
})

@romainmenke romainmenke marked this pull request as ready for review February 27, 2022 11:42
@romainmenke
Copy link
Collaborator Author

@RyanZim Thank you for the hint!

I think this is ready for review.
Please let me know if anything should be done differently or more test coverage is required.

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.

One question; otherwise looks good. BTW, sorry so long in getting around to reviewing this.

) {
return result.warn(
"@import must precede all other statements (besides @charset)",
"@import must precede all other statements (besides @charset or empty @layer)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not saying this is wrong, but can you show where the spec allows for empty @layer before @import?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://drafts.csswg.org/css-cascade-5/#at-import

Any @import rules must precede all other valid at-rules and style rules in a style sheet (ignoring @charset and empty @layer definitions) and must not have any other valid at-rules or style rules between it and previous @import rules, or else the @import rule is invalid.

https://drafts.csswg.org/css-cascade-5/#layer-block

Note: @layer block at-rules cannot be interleaved with @import rules.

In more detail : https://drafts.csswg.org/css-cascade-5/#layer-empty

The @layer rule can also be used to define new layers without assigning any style rules, by providing only the layer name:

@layer #;
Such empty @layer rules are allowed before @import and @namespace rules (after the @charset rule, if any) as well as everywhere @layer block at-rules are allowed.


But now that I have some distance since writing this I think that empty @layer is not a useful way to describe this to stylesheet authors.

I had to look it up, both to check that I didn't make a mistake and what the exact meaning was.

Is there a better, more widely used name for at-rules without a block?

@layer foo; /* a good name for this? */

@import 'baz.css' layer(baz);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if there's a good name for it; I think following the spec's wording is a safe bet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That works for me!
We can also see if this causes confusion to authors and adjust if needed.
It might be a non issue as people learn the feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@RyanZim Is this resolved or do you want me to change something?
Not in any rush to have this shipped, but wasn't sure :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

All set, just need to merge and release this when I get a chance.

@RyanZim RyanZim merged commit 8bd0173 into postcss:master Mar 22, 2022
@romainmenke romainmenke deleted the cascade-layers-support--ambitious-squid-0472bea492 branch March 22, 2022 15:01
@RyanZim
Copy link
Collaborator

RyanZim commented Mar 22, 2022

Just thought about it, perhaps there should be some mention of this in the README?

@romainmenke
Copy link
Collaborator Author

Yes!
I'll add it and open a new PR.

@RyanZim
Copy link
Collaborator

RyanZim commented Mar 22, 2022

In the meantime; released in v14.1.0! 🎉

@romainmenke
Copy link
Collaborator Author

Nice! Just used it to double check the example for the readme!

Thank you for all the reviews and guidance 🎉

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

Successfully merging this pull request may close these issues.

None yet

2 participants