build: update angular and dev-infra package while accounting for rule changes #44490

devversion · 2021-12-15T15:30:16Z

See individual commits

mary-poppins · 2021-12-15T18:34:15Z

You can preview a159198 at https://pr44490-a159198.ngbuilds.io/.
You can preview a757a9d at https://pr44490-a757a9d.ngbuilds.io/.

mary-poppins · 2021-12-15T18:40:45Z

You can preview 7d9a1cc at https://pr44490-7d9a1cc.ngbuilds.io/.

josephperrott

LGTM

Reviewed-for: global-approvers

mary-poppins · 2021-12-15T19:25:18Z

You can preview 59190c2 at https://pr44490-59190c2.ngbuilds.io/.

gkalpak

Overall lgtm 👍

One more question:

I noticed that the new rule makes the following changes (compared to the old rule) to the generated ngsw-worker.js:

Removes comments.
Removes /*@__PURE__*/ annotations
Downlevels 2016+ features, such as async/await, {...props}, ?. and ?? (I assume due to us targeting ES2015).

Are these changes intentional? (Why not target >ES2015, for example?)

modules/benchmarks/src/styling/ng2/init.ts

modules/benchmarks/src/tree/ng2_static/tree.ts

devversion · 2021-12-15T20:55:07Z

@gkalpak I think we accidentally shipped that file as ES2020 at some point. I think we should ship it as ES2015 (did that intentionally) since not all browsers support ES2020 fully, and the file is put into apps by the CLI without any downleveling/processing IIRC.

This also explains why the pure annotations etc should not matter I think. Especially since all of that is within a IIFE. These things are likely controlled by some esbuild options. The pure annotations were added by build optimizer IIRC (not even sure if that is expected)

What do you think? I'm not super familiar with the service worker code

devversion · 2021-12-16T12:48:49Z

For reference here is the simple copy logic in the CLI: https://cs.opensource.google/angular/angular-cli/+/master:packages/angular_devkit/build_angular/src/utils/service-worker.ts;l=115-120?q=ngsw-worker&ss=angular%2Fangular-cli. The pure annotations really were just added by build optimizer as a "side-effect" (ironically) of the benchmarking rule being used, or was that decision deliberately made?

gkalpak · 2021-12-16T15:43:28Z

I think we accidentally shipped that file as ES2020 at some point. I think we should ship it as ES2015 (did that intentionally) since not all browsers support ES2020 fully, and the file is put into apps by the CLI without any downleveling/processing IIRC.

TBH, I don't recall if it was intentional that we switched from ES2015 to ES2020 😅
AFAICT, all supported browsers would support at least ES2017 (see caniuse) and possibly even ES2019 (for the features we need - see caniuse). AFAICT, the CLI ships ES2017 by default anyway (with the exception of downleveling async/await for the Zone.js compatibility (which is not a concern for the SW).
So, I think we could safely target at least ES2017. Or am I missing something?
(BTW, not feeling strongly about this, so leaving it up to you - I just wanted to make sure that the changes were intentional.)

This also explains why the pure annotations etc should not matter I think.

Yes, you are right about the pure annotations. They shouldn't have any effect (at least not atm).

Finally, the comments were nice to preserve for debugging. Bundle size is little of a concern for the SW script, because (a) it is not on a critical path and (b) it is only fetched when it changes (which for the Angular SW is fairly infrequent).

Not a big issue, but if it is easy to preserve comments with esbuild, I think that would be nice. (If it is complicated, then it is perfectly fine to have them removed.)

(NOTE: If we are concerned about the bundle size, we should minify the script and provide sourcemaps for debugging. This is being tracked in #27490 btw.)

Updates Angular and the dev-infra package to the latest version/build.

…nctions Updates the symbol extractor to support IIFE bundles using arrow-functions instead of function declarations. This is in preparation for running symbol extraction tests with the overhauled optimization pipeline for Angular v13, relying on ESBuild internally. Also removes rollup-specific code that does not seem to be relevant anymore / rollup will be replaced anyway.

…y rule Adds the postinstall script as runfile for the `yarn_install` repository rule, so that the dependencies are re-fetched when the script changes.

…es in dev-infra We are in an inconvenient situation where the ng-dev package might rely on packages from the Angular framework repository. Given that we install this package in the framework repository, we need to update some references through a postinstall. This commit updates the patches to account for the latest changes in the dev-infra package/repository.

The `ng_rollup_bundle` rule has been replaced with a new rule called `app_bundle`. This rule replicates the Angular v13 optimization pipeline in the CLI, so that we can get better benchmarking results. Also the rule is much simpler to maintain as it relies on ESbuild. The old `ng_rollup_bundle` rule did rely on e.g. build-optimizer that no longer has an effect on v13 Angular packages, so technically size tests/symbol tests were no longer as correct as they were before. This commit fixes that. A couple of different changes and their explanation: * Language-service will no longer use the benchmark rule for creating its NPM bundles! It will use plain `rollup_bundle`. ESBuild would have been nice but the language-service relies on AMD that ESBuild cannot generate (yet?) * Service-worker ngsw-worker.js file was generated using the benchmark bundle rule. This is wrong. We will use a simple ESbuild rule in the future. The output is more predictable that way, and we can have a clear use of the benchmark bundle rule.. * A couple of benchmarks in `modules/` had to be updated to use e.g. `initTableUtils` calls. This is done because with the new rule, all files except for the entry-point are considered side-effect free. The utilities for benchmarks relied on side-effects in some transitively-loaded file (bad practice anyway IMO). We are now initializing the utilities using a proper init function that is exported...

The ng-dev release config changed its release configuration in order to support experimental packages. This commit updates the FW release config to work with the new config signature of ng-dev. All of our peackages are non-experimental, so we do not specify an explicit `experimental` property.

Updates the size goldens to the reflect the latest CLI devkit updates.

devversion · 2021-12-16T16:01:04Z

Addressed all feedback, and switched the worker target to ES2017 as discussed (added a comment about ES2019 to the target). I also looked into preserving the comments but ESBuild drops them when parsing code, so there is no way to keep them. Hopefully we can just ship a minfied version of the worker in the future, with proper source-maps!

mary-poppins · 2021-12-16T16:07:33Z

You can preview 127dafd at https://pr44490-127dafd.ngbuilds.io/.

dylhunn · 2022-01-04T20:14:18Z

This PR was merged into the repository by commit 3a29c57.

Updates Angular and the dev-infra package to the latest version/build. PR Close #44490

…nctions (#44490) Updates the symbol extractor to support IIFE bundles using arrow-functions instead of function declarations. This is in preparation for running symbol extraction tests with the overhauled optimization pipeline for Angular v13, relying on ESBuild internally. Also removes rollup-specific code that does not seem to be relevant anymore / rollup will be replaced anyway. PR Close #44490

…y rule (#44490) Adds the postinstall script as runfile for the `yarn_install` repository rule, so that the dependencies are re-fetched when the script changes. PR Close #44490

…es in dev-infra (#44490) We are in an inconvenient situation where the ng-dev package might rely on packages from the Angular framework repository. Given that we install this package in the framework repository, we need to update some references through a postinstall. This commit updates the patches to account for the latest changes in the dev-infra package/repository. PR Close #44490

…44490) The `ng_rollup_bundle` rule has been replaced with a new rule called `app_bundle`. This rule replicates the Angular v13 optimization pipeline in the CLI, so that we can get better benchmarking results. Also the rule is much simpler to maintain as it relies on ESbuild. The old `ng_rollup_bundle` rule did rely on e.g. build-optimizer that no longer has an effect on v13 Angular packages, so technically size tests/symbol tests were no longer as correct as they were before. This commit fixes that. A couple of different changes and their explanation: * Language-service will no longer use the benchmark rule for creating its NPM bundles! It will use plain `rollup_bundle`. ESBuild would have been nice but the language-service relies on AMD that ESBuild cannot generate (yet?) * Service-worker ngsw-worker.js file was generated using the benchmark bundle rule. This is wrong. We will use a simple ESbuild rule in the future. The output is more predictable that way, and we can have a clear use of the benchmark bundle rule.. * A couple of benchmarks in `modules/` had to be updated to use e.g. `initTableUtils` calls. This is done because with the new rule, all files except for the entry-point are considered side-effect free. The utilities for benchmarks relied on side-effects in some transitively-loaded file (bad practice anyway IMO). We are now initializing the utilities using a proper init function that is exported... PR Close #44490

…44490) The ng-dev release config changed its release configuration in order to support experimental packages. This commit updates the FW release config to work with the new config signature of ng-dev. All of our peackages are non-experimental, so we do not specify an explicit `experimental` property. PR Close #44490

Updates the size goldens to the reflect the latest CLI devkit updates. PR Close #44490

…nctions (#44490) Updates the symbol extractor to support IIFE bundles using arrow-functions instead of function declarations. This is in preparation for running symbol extraction tests with the overhauled optimization pipeline for Angular v13, relying on ESBuild internally. Also removes rollup-specific code that does not seem to be relevant anymore / rollup will be replaced anyway. PR Close #44490

…y rule (#44490) Adds the postinstall script as runfile for the `yarn_install` repository rule, so that the dependencies are re-fetched when the script changes. PR Close #44490

…es in dev-infra (#44490) We are in an inconvenient situation where the ng-dev package might rely on packages from the Angular framework repository. Given that we install this package in the framework repository, we need to update some references through a postinstall. This commit updates the patches to account for the latest changes in the dev-infra package/repository. PR Close #44490

…44490) The `ng_rollup_bundle` rule has been replaced with a new rule called `app_bundle`. This rule replicates the Angular v13 optimization pipeline in the CLI, so that we can get better benchmarking results. Also the rule is much simpler to maintain as it relies on ESbuild. The old `ng_rollup_bundle` rule did rely on e.g. build-optimizer that no longer has an effect on v13 Angular packages, so technically size tests/symbol tests were no longer as correct as they were before. This commit fixes that. A couple of different changes and their explanation: * Language-service will no longer use the benchmark rule for creating its NPM bundles! It will use plain `rollup_bundle`. ESBuild would have been nice but the language-service relies on AMD that ESBuild cannot generate (yet?) * Service-worker ngsw-worker.js file was generated using the benchmark bundle rule. This is wrong. We will use a simple ESbuild rule in the future. The output is more predictable that way, and we can have a clear use of the benchmark bundle rule.. * A couple of benchmarks in `modules/` had to be updated to use e.g. `initTableUtils` calls. This is done because with the new rule, all files except for the entry-point are considered side-effect free. The utilities for benchmarks relied on side-effects in some transitively-loaded file (bad practice anyway IMO). We are now initializing the utilities using a proper init function that is exported... PR Close #44490

…44490) The ng-dev release config changed its release configuration in order to support experimental packages. This commit updates the FW release config to work with the new config signature of ng-dev. All of our peackages are non-experimental, so we do not specify an explicit `experimental` property. PR Close #44490

Updates the size goldens to the reflect the latest CLI devkit updates. PR Close #44490

angular-automatic-lock-bot · 2022-02-04T16:11:06Z

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.}

devversion changed the title ~~Angular dev infra update~~ build: update angular and dev-infra package while accounting for rule changes Dec 15, 2021

devversion force-pushed the angular-dev-infra-update branch 5 times, most recently from 13209ed to 7d9a1cc Compare December 15, 2021 18:32

devversion added the aio: preview label Dec 15, 2021

devversion marked this pull request as ready for review December 15, 2021 18:52

pullapprove bot requested a review from alxhub December 15, 2021 18:52

devversion added action: review The PR is still awaiting reviews from at least one requested reviewer area: build & ci Related the build and CI infrastructure of the project target: patch This PR is targeted for the next patch release labels Dec 15, 2021

ngbot bot modified the milestone: Backlog Dec 15, 2021

devversion requested review from josephperrott and gkalpak December 15, 2021 18:53

josephperrott approved these changes Dec 15, 2021

View reviewed changes

devversion added the action: presubmit The PR is in need of a google3 presubmit label Dec 15, 2021

gkalpak approved these changes Dec 15, 2021

View reviewed changes

modules/benchmarks/src/styling/ng2/init.ts Show resolved Hide resolved

modules/benchmarks/src/tree/ng2_static/tree.ts Show resolved Hide resolved

devversion added 4 commits December 16, 2021 16:59

build: update angular

0799a17

Updates Angular and the dev-infra package to the latest version/build.

build: add postinstall script as runfile for yarn_install repositor…

02d5d83

…y rule Adds the postinstall script as runfile for the `yarn_install` repository rule, so that the dependencies are re-fetched when the script changes.

devversion added 3 commits December 16, 2021 16:59

test: update size goldens to reflect latest CLI devkit update

127dafd

Updates the size goldens to the reflect the latest CLI devkit updates.

devversion force-pushed the angular-dev-infra-update branch from 59190c2 to 127dafd Compare December 16, 2021 15:59

devversion added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 16, 2021

devversion removed the request for review from alxhub December 16, 2021 16:01

gkalpak approved these changes Dec 16, 2021

View reviewed changes

devversion removed the action: presubmit The PR is in need of a google3 presubmit label Dec 16, 2021

dylhunn pushed a commit that referenced this pull request Jan 4, 2022

build: update angular (#44490)

158c9d6

Updates Angular and the dev-infra package to the latest version/build. PR Close #44490

dylhunn pushed a commit that referenced this pull request Jan 4, 2022

test: update size goldens to reflect latest CLI devkit update (#44490)

46c6f20

Updates the size goldens to the reflect the latest CLI devkit updates. PR Close #44490

dylhunn closed this in 02bc458 Jan 4, 2022

dylhunn pushed a commit that referenced this pull request Jan 4, 2022

test: update size goldens to reflect latest CLI devkit update (#44490)

3a29c57

Updates the size goldens to the reflect the latest CLI devkit updates. PR Close #44490

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: update angular and dev-infra package while accounting for rule changes #44490

build: update angular and dev-infra package while accounting for rule changes #44490

devversion commented Dec 15, 2021

mary-poppins commented Dec 15, 2021

mary-poppins commented Dec 15, 2021

josephperrott left a comment

mary-poppins commented Dec 15, 2021

gkalpak left a comment

devversion commented Dec 15, 2021 •

edited

devversion commented Dec 16, 2021

gkalpak commented Dec 16, 2021

devversion commented Dec 16, 2021

mary-poppins commented Dec 16, 2021

dylhunn commented Jan 4, 2022

angular-automatic-lock-bot bot commented Feb 4, 2022

build: update angular and dev-infra package while accounting for rule changes #44490

build: update angular and dev-infra package while accounting for rule changes #44490

Conversation

devversion commented Dec 15, 2021

mary-poppins commented Dec 15, 2021

mary-poppins commented Dec 15, 2021

josephperrott left a comment

Choose a reason for hiding this comment

mary-poppins commented Dec 15, 2021

gkalpak left a comment

Choose a reason for hiding this comment

devversion commented Dec 15, 2021 • edited

devversion commented Dec 16, 2021

gkalpak commented Dec 16, 2021

devversion commented Dec 16, 2021

mary-poppins commented Dec 16, 2021

dylhunn commented Jan 4, 2022

angular-automatic-lock-bot bot commented Feb 4, 2022

devversion commented Dec 15, 2021 •

edited