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

feat(css): implement @layer and @import rules #15892

Closed

Conversation

noreiller
Copy link
Contributor

The changes add the support of @layer css at-rule and implements the rules when using @import CSS at-rule.

It is part of CSS experiment.

@layer
The following code would be handled by webpack, especially in CSS modules.

@layer base, special;

body {
  background: green;
  font-family: "Open Sans";
}

@layer special {
  .item {
    color: rebeccapurple;
  }
}

@layer base {
  .item {
    color: black;
    border: 5px solid black;
    font-size: 1.3em;
    padding: .5em;
  }
}

@import rules
This code:

/* style.css */
@import url( "style2.css" ) layer( base ) supports( font-weight: bold ) screen and (min-width: 1024px);

/* style2.css */
body {
  font-weight: bold;
  text-decoration: underline;
}

would be transformed to:

@layer base {
  @supports(font-weight: bold) {
    @media screen and (min-width: 1024px) {
      body {
        font-weight: bold;
        text-decoration: underline;
      }
    }
  }
}

Tests
Some tests have been added for CSS modules.
The ones dedicated to @import rules have also been added but, unfortunately, JSDOM (which relies on CSSOM) does not handle well at-rules so they are disabled until the support is updated.

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)


if (
// Do nothing more for external modules
/^(\/\/|https?:\/\/|std:)/.test(dep.request) ||
Copy link
Member

Choose a reason for hiding this comment

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

We can handle externals styles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

External styles are already considered as externals assets and so, no need to transform them. It's done there: https://github.com/webpack/webpack/blob/main/lib/WebpackOptionsApply.js#L119-L163.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, I forgot that. Thanks, I will check and update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexander-akait The code is updated with buildHttp experiment and also with assets replacements.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good to me

Comment on lines +75 to +103
const replacedSource = `${source.source()}`;

// We use the replaced source and reset the replacements as a patch for to replace all the code
// @ts-expect-error
source._replacements = [];

source.replace(
0,
source.size(),
`
${decorations
.map(([before, after], idx) => {
return Array(idx)
.fill()
.reduce(tpl => Template.indent(tpl), before);
})
.join("\n")}
${Array(decorations.length)
.fill()
.reduce(tpl => Template.indent(tpl), replacedSource)}
${decorations
.map(([before, after], idx) => {
return Array(decorations.length - 1 - idx)
.fill()
.reduce(tpl => Template.indent(tpl), after);
})
.join("\n")}`
);
}
Copy link
Member

Choose a reason for hiding this comment

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

replacing the whole source shouldn't be done here. It breaks all SourceMapping.
Instead insert the decorations before and after the existing code with source.insert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it aft first try but I add issues with source mapping. I think it's related to the issue you mentioned in another comment.

get type() {
return "css @import decorator";
}
}
Copy link
Member

Choose a reason for hiding this comment

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

It needs serialize and deserialize methods to take care of serialization of this.decorations

);
const decoratorDep = new CssImportDecoratorDependency(decorations);

dependencyModule.addDependency(decoratorDep);
Copy link
Member

Choose a reason for hiding this comment

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

You can't add dependencies during code generation. This need to be added during parsing.

Copy link
Member

Choose a reason for hiding this comment

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

And you should not add dependencies to other modules. So this need to be handled differently.

There is also the problem that a single module might be imported multiple times with different layer, supports and/or media.

Copy link
Member

Choose a reason for hiding this comment

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

So what need to be done here is that CssImportDependency need to have layer supports and media properties.
It then need to resolve to different CssModules depending on that. CssModule also need to have layer supports and media properties. These need to be inherited to imported module.

CssImportDependency need to override getResourceIdentifier() to add layer supports and media, because they influence resolving and dependencies are grouped based on that identifier.

CssImportDependency can't use the normalModuleFactory (in CssModulesPlugin). It need to have a custom factory that takes care of copying these properties from the dependency to the module.

I would also omit the decorator dependency at all and take care of adding the decorations in CssModulePlugin.renderChunk. Here you can also use the PrefixSource to apply indent in a SourceMap compatible way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @sokra, I rewrote the implementation. I hope it fits with what you said.

@@ -0,0 +1,112 @@
/*
MIT License http://www.opensource.org/licenses/mit-license.php
Author Ivan Kopeykin @vankop
Copy link
Member

Choose a reason for hiding this comment

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

You can put your name here

Comment on lines +420 to +472
pos++;
let cc = input.charCodeAt(pos);
while (_isWhiteSpace(cc)) {
pos++;
if (pos === input.length) return pos;
cc = input.charCodeAt(pos);
}
const contentStart = pos;
let contentEnd;
for (;;) {
while (_isWhiteSpace(cc)) {
pos++;
if (pos === input.length) return pos;
cc = input.charCodeAt(pos);
}
if (cc === CC_RIGHT_PARENTHESIS) {
if (level > 0) {
level -= 1;
pos++;
} else {
contentEnd = pos;
let previousPos = pos - 1;
let previousCc = input.charCodeAt(previousPos);
while (_isWhiteSpace(previousCc)) {
contentEnd -= 1;
previousPos -= 1;
previousCc = input.charCodeAt(previousPos);
}
pos++;
if (callbacks.supports !== undefined) {
return callbacks.supports(
input,
start,
pos,
contentStart,
contentEnd
);
}
return pos;
}
} else if (cc === CC_LEFT_PARENTHESIS) {
level += 1;
pos++;
} else {
pos++;
}
if (pos === input.length) return pos;
cc = input.charCodeAt(pos);
}
Copy link
Member

Choose a reason for hiding this comment

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

supports and layer share a lot of code. Could we put that into a shared function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will refactor that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@noreiller
Copy link
Contributor Author

@sokra Thanks for the review, I will change the proposal according to what you asked but it won't be until a few weeks since I have some holidays.
In the mean time, if you can check other PRs related to Webpack CSS (#15812 #15818), I would be really thankful.

@noreiller
Copy link
Contributor Author

Hello @sokra @alexander-akait, will you continue to implement the webpack experimental CSS feature or accept new PRs?

@alexander-akait
Copy link
Member

@noreiller Let's do rebase, thank you

@webpack-bot
Copy link
Contributor

@noreiller Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@alexander-akait Please review the new changes.

@alexander-akait
Copy link
Member

Thank you I will review soon

@noreiller noreiller force-pushed the css-at-import-rules-and-at-layer branch from fa8bd27 to 8af5b74 Compare April 14, 2023 07:12
@alexander-akait
Copy link
Member

After merge - #16978 (because it fixed some bugs in tokenizer), I will focus on it, I have idea how we can simplify it

@noreiller
Copy link
Contributor Author

After merge - #16978 (because it fixed some bugs in tokenizer), I will focus on it, I have idea how we can simplify it

It's gonna be another complicated rebase... 😮
I'll wait for your merge and then take care of it.

@alexander-akait
Copy link
Member

@noreiller don't worry, you have

Maintainers are allowed to edit this pull request.

So I will do rebase

@noreiller
Copy link
Contributor Author

@noreiller don't worry, you have

Maintainers are allowed to edit this pull request.

So I will do rebase

Ok, thanks 🙏

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.

None yet

4 participants