-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
build: update size test tool to work with Angular v13 #23811
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
28137a4
test: remove unnecessary boostrap module calls in size tests
devversion c6c5d23
build: add comment explaining the processing of `.mjs` in esbuild rule
devversion 6c0e4af
build: update size test tool to work with Angular v13
devversion 9a5cef9
test: update size-golden to reflect recent compilation pipeline changes
devversion File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,18 @@ | ||
cdk/drag-drop/all-directives: 160859 | ||
cdk/drag-drop/basic: 158225 | ||
material-experimental/mdc-chips/basic: 385551 | ||
material-experimental/mdc-form-field/advanced: 417584 | ||
material-experimental/mdc-form-field/basic: 416339 | ||
material/autocomplete/without-optgroup: 392028 | ||
material/button-toggle/standalone: 124412 | ||
material/chips/basic: 320073 | ||
material/datepicker/range-picker/without-form-field: 505044 | ||
material/expansion/without-accordion: 330526 | ||
material/form-field/advanced: 377468 | ||
material/form-field/basic: 376144 | ||
material/list/nav-list: 328072 | ||
material/menu/without-lazy-content: 398590 | ||
material/radio/without-group: 127571 | ||
material/select/basic: 437305 | ||
material/tabs/advanced: 369608 | ||
material/tabs/basic: 368747 | ||
cdk/drag-drop/all-directives: 155091 | ||
cdk/drag-drop/basic: 152522 | ||
material-experimental/mdc-chips/basic: 249660 | ||
material-experimental/mdc-form-field/advanced: 296409 | ||
material-experimental/mdc-form-field/basic: 294855 | ||
material/autocomplete/without-optgroup: 281544 | ||
material/button-toggle/standalone: 186619 | ||
material/chips/basic: 228157 | ||
material/datepicker/range-picker/without-form-field: 398783 | ||
material/expansion/without-accordion: 200184 | ||
material/form-field/advanced: 247973 | ||
material/form-field/basic: 246355 | ||
material/list/nav-list: 194468 | ||
material/menu/without-lazy-content: 286831 | ||
material/radio/without-group: 189803 | ||
material/select/basic: 329893 | ||
material/tabs/advanced: 248653 | ||
material/tabs/basic: 247787 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
/** | ||
* @license | ||
* Copyright Google LLC All Rights Reserved. | ||
* | ||
* Use of this source code is governed by an MIT-style license that can be | ||
* found in the LICENSE file at https://angular.io/license | ||
*/ | ||
|
||
import babel from '@babel/core'; | ||
import {createEs2015LinkerPlugin} from '@angular/compiler-cli/linker/babel'; | ||
import {ConsoleLogger, NodeJSFileSystem, LogLevel} from '@angular/compiler-cli'; | ||
import {GLOBAL_DEFS_FOR_TERSER_WITH_AOT} from '@angular/compiler-cli/private/tooling'; | ||
import adjustStaticClassMembersPlugin from '@angular-devkit/build-angular/src/babel/plugins/adjust-static-class-members.js'; | ||
import elideAngularMetadataPlugin from '@angular-devkit/build-angular/src/babel/plugins/elide-angular-metadata.js'; | ||
import adjustTypeScriptEnumsPlugin from '@angular-devkit/build-angular/src/babel/plugins/adjust-typescript-enums.js'; | ||
import pureToplevelFunctionsPlugin from '@angular-devkit/build-angular/src/babel/plugins/pure-toplevel-functions.js'; | ||
import fs from 'fs'; | ||
|
||
/** Babel plugin running the Angular linker. */ | ||
const linkerBabelPlugin = createEs2015LinkerPlugin({ | ||
fileSystem: new NodeJSFileSystem(), | ||
logger: new ConsoleLogger(LogLevel.warn), | ||
linkerJitMode: false, | ||
}); | ||
|
||
/** | ||
* ESBuild plugin configuring various optimization Babel plugins. The Babel plugins | ||
* configured as part of this plugin run in the Angular CLI compilation pipeline as well. | ||
*/ | ||
const esbuildBabelOptimizePlugin = { | ||
name: 'ng-babel-optimize-esbuild', | ||
setup: build => { | ||
build.onLoad({filter: /.*/}, async args => { | ||
const filePath = args.path; | ||
const content = await fs.promises.readFile(filePath, 'utf8'); | ||
const plugins = [ | ||
linkerBabelPlugin, | ||
adjustStaticClassMembersPlugin, | ||
elideAngularMetadataPlugin, | ||
adjustTypeScriptEnumsPlugin, | ||
]; | ||
|
||
// All files except for the auto-generated module entry-point are considered side-effect | ||
// free. For these we can add the pure-top level Babel plugin. This matches conceptually | ||
// with what is done in the Angular CLI compilation pipeline, with respect to everything | ||
// in this repo being an official side-effect free APF package. | ||
if (!args.path.includes('autogenerated_module_index.mjs')) { | ||
plugins.push(pureToplevelFunctionsPlugin); | ||
} | ||
|
||
const {code} = await babel.transformAsync(content, { | ||
filename: filePath, | ||
filenameRelative: filePath, | ||
plugins: plugins, | ||
// Sourcemaps are generated inline so that ESBuild can process them. | ||
sourceMaps: 'inline', | ||
compact: false, | ||
}); | ||
|
||
return {contents: code}; | ||
}); | ||
}, | ||
}; | ||
|
||
export default { | ||
// Note: We prefer `.mjs` here as this is the extension used by Angular APF packages. | ||
resolveExtensions: ['.mjs', '.js'], | ||
conditions: ['es2020', 'es2015'], | ||
mainFields: ['fesm2020', 'es2020', 'es2015', 'module'], | ||
format: 'iife', | ||
// The majority of these options match with the ones the CLI sets: | ||
// https://github.com/angular/angular-cli/blob/0d76bf04bca6e083865972b5398a32bbe9396e14/packages/angular_devkit/build_angular/src/webpack/plugins/javascript-optimizer-worker.ts#L133. | ||
treeShaking: true, | ||
minifyIdentifiers: true, | ||
minifySyntax: true, | ||
minifyWhitespace: false, | ||
pure: ['forwardRef'], | ||
legalComments: 'none', | ||
// ESBuild requires the `define` option to take a string-based dictionary. | ||
define: convertObjectToStringDictionary(GLOBAL_DEFS_FOR_TERSER_WITH_AOT), | ||
plugins: [esbuildBabelOptimizePlugin], | ||
}; | ||
|
||
/** Converts an object to a string dictionary. */ | ||
function convertObjectToStringDictionary(value) { | ||
return Object.entries(value).reduce((result, [propName, value]) => { | ||
result[propName] = String(value); | ||
return result; | ||
}, {}); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,12 @@ | ||
{ | ||
"output": { | ||
"ecma": "es2015", | ||
"comments": false | ||
}, | ||
"ecma": "es2020", | ||
"compress": { | ||
"global_defs": { | ||
"ngDevMode": false, | ||
"ngI18nClosureMode": false, | ||
"ngJitMode": false | ||
}, | ||
"passes": 3, | ||
"passes": 2, | ||
"pure_getters": true | ||
}, | ||
"toplevel": true, | ||
"mangle": true | ||
"format": { | ||
"ascii_only": true, | ||
"wrap_func_args": false | ||
}, | ||
"mangle": false | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is going to go out of sync with the CLI eventually. Wouldn't it be better to run an actual CLI build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that this can become outdated in some cases, but usually the JS optimization pipeline in the CLI does not change too often and we just need to update this in some cases (like for v13).
The framework also runs tests like that without the CLI because a CLI project is much harder to integrate with size trackings in Bazel, and it wouldn't allow us to have such fine-grained and isolated scenario tests (notice how we have various scenarios like drag drop basic, advanced etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subjectively I also think that a pipeline like that allows for easier debugging. In the CLI it is a little more difficult to follow what caused a problem, while here it's a rather simple script allowing for easier/better reasoning and manual edits for debugging (extremely helpful when I compared MDC sizes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the ease of debugging really relevant when we can't trust that the setup is the same as for real CLI users?
I don't see why we wouldn't be able to have the same granularity in the test scenarios either. We could get away with defining a single CLI workspace and then each test scenario would be a
project
within the sameangular.json
. That would allow us to build everything with a single command and we might not even need to go through Bazel.Also I believe that the FW repo has CLI-based size tests that run against AIO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely is, at least for me. I added these size checks initially to debug the lightweight token Ivy issue, and also used it to compare MDC sizes. For that, it was significantly easier inspecting the non-Webpack convoluted bundle, while I could also disable certain optimizations in a fine-grained way. i.e. disabling mangling, beautifying output, keeping pure calls etc. Some of this is also do-able with the CLI but only with some manual edits of the node modules (some things can also be controlled with unknown/internal environment variables though)
Yeah, but this is significantly more code to maintain, starting with the generation of all the other unused parts of a CLI application, and having to maintain the angular.json file..
We heavily invested in running everything in Bazel, especially in the FW repo. Going through Bazel makes it easier to run these things consistently on all platforms (in a hermetic way). The major benefit is also that the required NPM package output can be added as dependency from Bazel. This removes the need of copying package output outside of Bazel + enables caching, and remote execution.
yeah, they have, but it's significantly more difficult to maintain these within Bazel (there are a few CLI Bazel size tests in FW). A large problem would also be that all size tests run whenever something changes, while with the current approach only affected tests re-run. Lastly, it would require even more work to capture sizes if there are multi-projects (for all the scenarios) in a single CLI workspace.
Happy to discuss this further. also cc. @josephperrott for this
Generally I'd also prefer to rely as much as possible on the CLI here, but we kind of do leverage as much as possible with my current approach, without all the downsides of using the CLI directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I think the discussion of whether this should run with the CLI or not is not in-scope for this PR. We can/should discuss this more though.
For now, this is low-maintenace and makes our size tests work properly for v13.