Skip to content

Conversation

@layershifter
Copy link
Member

@layershifter layershifter commented Nov 23, 2022

This PR refactors Webpack plugin for CSS extraction.

Before

We use the same approach as CompiledCSS: we create a new cacheGroup in optimization.splitChunks and force all generated CSS files by us to go there.

As a result we will get a single asset (CSS file) that we can process and sort rules. This works, but this approach requires optimization.splitChunks to be enabled. One of our key partners has this setting disabled in dev environment: all try outs to enable it were not successful.

This approach also prevents all attempts to implement chunks splitting support and avoid a single CSS file.

After

TL;DR Does the same, works differently ¯_(ツ)_/¯

The key difference is that we don't use optimization.splitChunks and moving CSS files to a target chunk.

Initial attempt

Inspired by atlassian-labs/compiled#724. All processing was done during .processAssets stage and it worked: we got a CSS file per a chunk (if it contained makeStyles calls) and I was able to extract rules from it to a dedicated chunk.

But:

  • If a chunk contained only Griffel's CSS after processing we got an empty file, for example:
    - griffel.css (contains all CSS)
    - chunk.css (became empty file)
    - async-chunk.css (became empty file)
    
  • These empty files could not be removed (via compilation.deleteAsset()) as mini-css-extract-plugin will register async chunks in JS runtime and these assets will fail to load.
  • Whatever modifications to assets we will do - we should do them before .runtimeRequirementInTree (undocumented Webpack hook) otherwise chunks that have empty CSS could be registered in JS runtime 💣

Implementation in this PR

After multiple attempts to do something with assets I realized that .processAssets is too late to transfer CSS between chunks. The problem that at any stage before .runtimeRequirementInTree we don't have assets - we operate with modules (not really well documented). Simple explanation: an asset is a concatenated set of modules.

On .afterChunks (again undocumented, happens after a chunk graph gets generated and modules were collected):

  • we attach a custom chunk to main entrypoint
  • we move all modules produced by mini-css-extract-plugin to that chunk (if they contain CSS produced by Griffel)

On .processAssets we are in the same condition actually as currently: all CSS modules produced by Griffel were moved to a dedicated chunk -> this chunk has a single CSS file -> we sort rules in it ✅

@github-actions
Copy link

github-actions bot commented Nov 23, 2022

📊 Bundle size report

🤖 This report was generated against e242ccaaa99b08bbb0cfdfdb9fcd2db802c33fc2

@layershifter layershifter force-pushed the chore/rework-css-extraction branch 3 times, most recently from 4e9ed2a to 35cb4c8 Compare November 24, 2022 17:59
@layershifter layershifter force-pushed the chore/rework-css-extraction branch from 35cb4c8 to 86554e5 Compare November 24, 2022 18:20
"root": "e2e/nextjs",
"sourceRoot": "e2e/nextjs/src",
"projectType": "library",
"implicitDependencies": ["@griffel/webpack-loader"],
Copy link
Member Author

Choose a reason for hiding this comment

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

As we don't import/require @griffel/webpack-loader anywhere - Nx fails to detect it as a dependency.

"targets": {
"test": {
"executor": "@nrwl/workspace:run-commands",
"dependsOn": [{ "target": "build", "projects": "dependencies" }],
Copy link
Member Author

Choose a reason for hiding this comment

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

Have been in a wrong spot 🤦

@layershifter layershifter marked this pull request as ready for review November 24, 2022 18:35
@layershifter layershifter requested a review from a team as a code owner November 24, 2022 18:35
throw new Error('Failed to find and entry points in "compilation.entrypoints"');
}

const mainEntryPoint = entryPoints[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

is the main entrypoint guaranteed to be the first entrypoint in compilation.entrypoints ?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, what if webpack is configured with multiple entrypoints?

Copy link
Member Author

@layershifter layershifter Nov 25, 2022

Choose a reason for hiding this comment

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

It's a good question. In all scenarios that I currently tested attaching to main entrypoint was enough. So, I don't have answer if it's good or bad. Currently works.

@layershifter layershifter merged commit f5c9ec0 into microsoft:main Nov 30, 2022
@layershifter layershifter deleted the chore/rework-css-extraction branch November 30, 2022 09:04
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.

2 participants