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

[optimizer][scss] Webpack bundles/repeats potentially unnecessary code with each file #102829

Open
clintandrewhall opened this issue Jun 21, 2021 · 15 comments
Labels
discuss Team:Operations Team label for Operations Team Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins.

Comments

@clintandrewhall
Copy link
Contributor

clintandrewhall commented Jun 21, 2021

Today, the optimizer webpack.config prepends SCSS files with mixins, variables and other pre-requisites a SCSS file might utilize. As a result, e Every compiled SCSS file is bloated with an additional ~3k of code whether it needs it or not in order for webpack to append the CSS as a style element to the DOM. This is compounded by the number of themes we support. The impact of this is better described in #102822 but in raw, locally-generated builds, combining six small SCSS file imports into a single index.scss file had a *9% reduction in bundle size:

(Before on left, after on right)

Screen Shot 2021-06-21 at 5 13 35 PM

Screen Shot 2021-06-21 at 4 01 55 PM

I was chatting about this with @tylersmalley, and it appears the only preventative measure we have to prevent the duplication of this code is for plugins to combine their SCSS imports into a single SCSS file. Since Canvas (and Kibana) is starting to move to CSS-in-JS, this might be an acceptable short-to-medium-term solution.

EDIT: The results from #102822 are in:

Moving 8 imports to the index.scss file reduced the bundle size by 21k and removed 63 modules.

Screen Shot 2021-06-21 at 6 44 57 PM

@clintandrewhall clintandrewhall added discuss Team:Operations Team label for Operations Team labels Jun 21, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@spalger
Copy link
Contributor

spalger commented Jun 21, 2021

This @import that we put at the top of each scss file is only supposed to initialize some variables and mixins in the scss, it's not supposed to include any actual CSS unless the mixins are used by the styles... Sounds like a bug we should fix, as we're still going to need the variables available everywhere, no?

@clintandrewhall
Copy link
Contributor Author

In our case, since all of the SCSS files are imported by a single SCSS file, that import only happens once, so all of the vars and mixins are imported.

I'm not seeing the dedupe or removal of unused SCSS in the local dev build. Perhaps that's handled in the prod build? I guess we'll see when the PR finishes, but that would be awesome.

I wish there was a way to get prod webpack profiles of your plugin locally, easily... I'm always afraid I'm chasing a red herring.

@spalger
Copy link
Contributor

spalger commented Jun 21, 2021

dedupe or removal of unused SCSS

Best I can tell the scss behind this @import is all $variable: x, @mixin, and @function definitions which don't actually produce any CSS so there isn't any deduplication to do...

I wish there was a way to get prod webpack profiles of your plugin locally

Use --dist --profile and it will do just that.

@clintandrewhall
Copy link
Contributor Author

clintandrewhall commented Jun 21, 2021

@spalger I updated the issue with the results from the PR: less 21k and 63 fewer modules.

@clintandrewhall
Copy link
Contributor Author

clintandrewhall commented Jun 22, 2021

I did a local --dist build against master and confirmed the output. While the stats.json file is too large I created a gist of canvas.chunk.5.js.

All of the portions to the right are different builds of the individual SCSS files, with supporting code.

Screen Shot 2021-06-21 at 7 15 49 PM

@tylersmalley tylersmalley added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. labels Jun 22, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-design (Team:Kibana-Design)

@tylersmalley
Copy link
Contributor

I am also tagging Core and Design on this issue. Let me know if this is incorrect.

The sass-loader is prepending an import of src/core/public/core_app/styles/_globals_${theme}.scss which mostly imports from eui.

I am not sure if there is anything we can do here, other than creating documentation to suggest plugins don't liberally import SCSS into their components and instead limit the total count of imports.

@spalger
Copy link
Contributor

spalger commented Jun 22, 2021

Okay, I've dug into the bundles and while your assertion that there is extra overhead caused by using separate CSS imports is correct your assignment of blame to the @import in the sass-loader is incorrect. The issue is that every webpack CSS module needs a loader to put that string into the dom as a style element, and that results in a decent amount of code added to the bundle for each module:

image

To make things worse every CSS module is actually in the bundle 4 times, one for each theme, so we end up with a lot of overhead caused by the style-loader mechanism. Looking at this code it's really very small, but the fact that we're including v7 light and dark themes in the distributable is definitely compounding this error.

I'd like to look into #102930 to see if we can mitigate this overhead a little rather than recommend that teams unify their CSS.

We also don't want all CSS in one download so that it's always downloaded and applied, but breaking the CSS up a ton causes a lot of overhead, especially if your CSS is small.

@clintandrewhall
Copy link
Contributor Author

Thanks for the info, @spalger ... I updated the description of the issue to reflect this.

@clintandrewhall clintandrewhall changed the title [optimizer][scss] Webpack bundles/repeats potentially unnecessary SCSS code with each file [optimizer][scss] Webpack bundles/repeats potentially unnecessary code with each file Jun 22, 2021
@mshustov
Copy link
Contributor

Looking at this code it's really very small, but the fact that we're including v7 light and dark themes in the distributable is definitely compounding this error.

I haven't tested, but maybe we can gain some improvements with https://github.com/webpack-contrib/style-loader#singletonstyletag ?

Another idea: is there an equivalent of tslib for webpack?

@Dosant
Copy link
Contributor

Dosant commented Jul 30, 2021

I am curious if this is the same issue I noticed or if I should create a separate issue?
I was breaking an existing plugin and in a new plugin I ended up with a single line of scss that I should import in a plugin as import 'index.scss'

In bundle analyzer I noticed:

  • style-loader/dist/runtime
  • css-loader/dist/runtime
bundle screenshot

Screen Shot 2021-07-30 at 11 31 41

Should at least those loaders be a part of @kbn/ui-shared-deps?

@spalger
Copy link
Contributor

spalger commented Aug 2, 2021

@Dosant We could probably move the runtimes into externals, though I'm not 100% sure they're compatible (I think I tried this but found an issue which indicated it's not possible) but I do think it's a separate issue. This is more about the overhead created to call the runtime with unique options for each CSS/SCSS module.

@tylersmalley tylersmalley added 1 and removed 1 labels Oct 11, 2021
@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Oct 12, 2021
@tylersmalley tylersmalley removed loe:small Small Level of Effort impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. EnableJiraSync labels Mar 16, 2022
@pgayvallet pgayvallet removed the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Jul 5, 2024
@pgayvallet
Copy link
Contributor

Unassigning Core as this seems only related to our bundling system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Team:Operations Team label for Operations Team Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins.
Projects
None yet
Development

No branches or pull requests

7 participants