Skip to content

Conversation

alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented Jul 19, 2019

It’s very easy to inadvertently import toplevel css in component styles. Since component css is standalone and self-contained, it will never be shared between components and remains as a single large bundle for each component. This in turn adds a large amount of code that must be processed and increases bundle size.

Note: this will only work for AOT compilations

Related to: TOOL-949

@alan-agius4 alan-agius4 added the target: major This PR is targeted for the next major release label Jul 19, 2019
@alan-agius4 alan-agius4 requested a review from filipesilva July 19, 2019 10:16
At the moment child compilation warnings and errors are being lost as they are not passed to the root compilation.  This means that any errors that are being set by clean-css for component styles are being lost.
…styles

It’s very easy to inadvertently import toplevel css in component styles. Since component css is standalone and self-contained, it will never be shared between components and remains as a single large bundle for each component. This in turn adds a large amount of code that must be processed and increases bundle size.

Related to: TOOL-949
Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of questions.

const overrides = {
aot: true,
optimization: true,
budgets: [{ type: 'anyComponentStyle', maximumWarning: '1b' }],
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the plan for showing this warning overall? Will we update bundle budgets in 9 for all projects?

Copy link
Collaborator Author

@alan-agius4 alan-agius4 Jul 22, 2019

Choose a reason for hiding this comment

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

I think the best option would be to do it for 9 for existing projects since users won't typically run migrations on minor and for new projects we can do it right away.

  1. What should be the value maximumWarning and maximumError for new projects?
  2. I am guessing that for the migration we will not want to add maximumError as this will break exiting projects, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think it should be for 9 too. We should add the migration to this PR as well.

As for the default numbers, I'm not really sure. UTF-8 uses 1 to 4 bytes for each char, most commonly only 1 byte for US ASCII chars. So 1kb would be 1024 chars. Off the top of my head warning on 5kb and erroring on 20kb sounds within the realm of reasonable. @mgechev do you have a recommendation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ill start working on the migrations, and than we just change the number when we have more feedback from @mgechev.

Copy link
Member

Choose a reason for hiding this comment

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

A good source for setting up good defaults will be http://bit.ly/perf-budget-calculator

I'd recommend bumping the error limit up with few extra 100s of KBs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alan-agius4 do we have a way forward with a global css bundle budget? Not asking to have it included in this PR, just asking if the changes in this PR would get in the way of that. I imagine a anyGlobalCss budget or something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Global styles meaning styles in general or that are set globally ie imported via style.css?

For the later we already have the AnyCalculator which can be used to set budgets on global css.

However, if we'd want to add a budget for css within the component + global css we can do it and this PR will provide a way forward for that.

My concerns would be that if want to sum up css from all the components and global stylesheet, you might end up with a misleading budget. Let's say the result of total CSS is 500Kb, when having lazy loading this will never be loaded/parsed at once.

Copy link
Member

Choose a reason for hiding this comment

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

Dynamic budgets, such as LightWallet would be great in such scenario. In the same time, we might be able to estimate what would be loaded initially, right? In webpack we have this metadata in the compilation stats.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But this is indeed is the enabler for that :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We already have some budgets for initial chunks

Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

LGTM, just waiting for the final values for the budgets so marking it as blocked.

@vikerman vikerman merged commit 2a0dc39 into angular:master Jul 25, 2019
@alan-agius4 alan-agius4 deleted the css-component-bundle-budget branch July 25, 2019 04:27
alxhub pushed a commit to angular/angular that referenced this pull request Aug 2, 2019
alxhub pushed a commit to angular/angular that referenced this pull request Aug 2, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants