-
Notifications
You must be signed in to change notification settings - Fork 11.9k
feat(@angular-devkit/build-angular): add bundle budget for component styles #15127
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
Changes from all commits
39f1be0
d7cbf44
de71065
fdc54c2
95af6ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1024,6 +1024,7 @@ | |
"allScript", | ||
"any", | ||
"anyScript", | ||
"anyComponentStyle", | ||
"bundle", | ||
"initial" | ||
] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -482,6 +482,7 @@ | |
"allScript", | ||
"any", | ||
"anyScript", | ||
"anyComponentStyle", | ||
"bundle", | ||
"initial" | ||
] | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -64,6 +64,68 @@ describe('Browser Builder bundle budgets', () => { | |||
await run.stop(); | ||||
}); | ||||
|
||||
it(`shows warnings for large component css when using 'anyComponentStyle' when AOT`, async () => { | ||||
const overrides = { | ||||
aot: true, | ||||
optimization: true, | ||||
budgets: [{ type: 'anyComponentStyle', maximumWarning: '1b' }], | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this is indeed is the enabler for that :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have some budgets for initial chunks Line 62 in 47c0db9
|
||||
}; | ||||
|
||||
const cssContent = ` | ||||
.foo { color: white; padding: 1px; } | ||||
.buz { color: white; padding: 2px; } | ||||
.bar { color: white; padding: 3px; } | ||||
`; | ||||
|
||||
host.writeMultipleFiles({ | ||||
'src/app/app.component.css': cssContent, | ||||
'src/assets/foo.css': cssContent, | ||||
'src/styles.css': cssContent, | ||||
}); | ||||
|
||||
const logger = new logging.Logger(''); | ||||
const logs: string[] = []; | ||||
logger.subscribe(e => logs.push(e.message)); | ||||
|
||||
const run = await architect.scheduleTarget(targetSpec, overrides, { logger }); | ||||
const output = await run.result; | ||||
expect(output.success).toBe(true); | ||||
expect(logs.length).toBe(2); | ||||
expect(logs.join()).toMatch(/WARNING.+app\.component\.css/); | ||||
await run.stop(); | ||||
}); | ||||
|
||||
it(`shows error for large component css when using 'anyComponentStyle' when AOT`, async () => { | ||||
const overrides = { | ||||
aot: true, | ||||
optimization: true, | ||||
budgets: [{ type: 'anyComponentStyle', maximumError: '1b' }], | ||||
}; | ||||
|
||||
const cssContent = ` | ||||
.foo { color: white; padding: 1px; } | ||||
.buz { color: white; padding: 2px; } | ||||
.bar { color: white; padding: 3px; } | ||||
`; | ||||
|
||||
host.writeMultipleFiles({ | ||||
'src/app/app.component.css': cssContent, | ||||
'src/assets/foo.css': cssContent, | ||||
'src/styles.css': cssContent, | ||||
}); | ||||
|
||||
const logger = new logging.Logger(''); | ||||
const logs: string[] = []; | ||||
logger.subscribe(e => logs.push(e.message)); | ||||
|
||||
const run = await architect.scheduleTarget(targetSpec, overrides, { logger }); | ||||
const output = await run.result; | ||||
expect(output.success).toBe(false); | ||||
expect(logs.length).toBe(2); | ||||
expect(logs.join()).toMatch(/ERROR.+app\.component\.css/); | ||||
await run.stop(); | ||||
}); | ||||
|
||||
describe(`should ignore '.map' files`, () => { | ||||
it(`when 'bundle' budget`, async () => { | ||||
const overrides = { | ||||
|
Uh oh!
There was an error while loading. Please reload this page.