Resolve differences between dev-infra files in master and patch #37778
Conversation
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
josephperrott
added
action: merge
devversion
reviewed
Jun 29, 2020
devversion
added
the
action: cleanup
…gular#37595) Adds support for a caretaker note label to the merge script. Whenever a configured label is applied, the merge script will not merge automatically, but instead prompt first in order to ensure that the caretaker paid attention to the manual caretaker note on the PR. This helps if a PR needs special attention. PR Close angular#37595
We added a new dependency on `fs-extra` to the dev-infra package. We can remove this dependency and replace it with `shelljs` that is extensively used in other places already. The motiviation is that we can reduce dependencies needed for for consumption of the shared dev-infra package.
Cleans up the dependencies used in the shared dev-infra package configuration. With the recent benchmarking utilities that have been added, a lot of peer dependencies have been added. We decided that we don't want to list every used dependencies as peer dependency as that could result in unnecessary churn/noise for consumers of the dev-infra package. Additionally, not all parts of the dev-infra package are necessarily used. Due to this, we want to apply the following rules for the package dependencies: 1. If a dependency is only used in a shipped Bazel macro/rule that can be optionally consumed, omit it from `package.json`. Bazel reports the missing dependency on its own, so we want to avoid adding it to the package json file. 2. Otherwise, if the dependency is large and commonly used (like buildifier), add it to the `peerDependencies`. If not, add it to the dependencies that are always brought in. We consider it as acceptable to bring in a few small dependencies that might not be used or not. Making all of those option would complicate the use of the dev-infra package. ds
…bClient GitClient now uses GithubClient for github API interactions. GithubClient is a class which extends Octokit and provides a member which allows for GraphQL requests against the Github GraphQL api, as well as providing convenience methods for common/repeated Github API requests.
…output As of Angular Package Format v10, we no longer ship a `fesm5` and `fesm5` output in packages. We made this change to the `ng_package` rule but intentionally did not clean up related build actions. This follow-up commit cleans this up by: * No longer building fesm5 bundles, or providing esm2015 output. * No longer requesting and building a third flavor for ESM5. We can use TSC to downlevel ES2015 sources/prodmode output similarly to how it is done in `ng-packagr`. The third output flavor (ESM5) resulted in a build slow-down as we required a full recompilation of sources. Now, we only have a single compilation for prodmode output, and then downlevel it on-demand to ES5 for the UMD bundles. Here is timing for building the release packages in `angular/angular` before this change, and afterwards: * Before: 462.157s = ~7.7min * After: 339.703s = ~5.6min This signifies a time reduction by 27% when running `./scripts/build/build-packages-dist.sh`.
Adds the `LinkablePackageInfo` to the `ng_module` rule. This allows the linker to properly link `ng_module` targets in Node runtime actions. Currently this does not work properly and packages like `@angular/core` are not linked, so we cannot rely on the linker. https://github.com/bazelbuild/rules_nodejs/blob/9a5de3728b05bf1647bbb87ad99f54e626604705/internal/linker/link_node_modules.bzl#L144-L146.
…llup` Refactors the `ng_rollup_bundle` rule to a macro that relies on the `@bazel/rollup` package. This means that the rule no longer deals with custom ESM5 flavour output, but rather only builds prodmode ES2015 output. This matches the common build output in Angular projects, and optimizations done in CLI where ES2015 is the default optimization input. The motiviation for this change is: * Not duplicating rollup Bazel rules. Instead leveraging the official rollup rule. * Not dealing with a third TS output flavor in Bazel.The ESM5 flavour has the potential of slowing down local development (as it requires compilation replaying) * Updating the rule to be aligned with current CLI optimizations. This also _fixes_ a bug that surfaced in the old rollup bundle rule. Code that is unused, is not removed properly. The new rule fixes this by setting the `toplevel` flag. This instructs terser to remove unused definitions at top-level. This matches the optimization applied in CLI projects. Notably the CLI doesn't need this flag, as code is always wrapped by Webpack. Hence, the unused code eliding runs by default.
It looks like there is a leftover golden in the `ng_package` tests that is no longer used anywhere and does not reflect the latest Angular Package Format v10 changes. We should be able to remove it to keep our codebase healthy.
The language-service package currently sets the `module` `package.json` property and refers to a folder called `fesm5`. The language-service though does not build with `ng_package` so this folder never existed. Now with APF v10, ng package would not generate this folder either. We should just remove the property as the primary entry-point is the UMD bundle resolved through `main`. There is no module flavour exposed to the NPM package as `pkg_npm` uses the named AMD module devmode output that doesn't work for `module`.
Updates to the latest commit of the `angular/components` repository. We need to do this because we removed the `esm5.bzl` output flavour aspect, but an old version of the components repo relied on this file to exist. This is no longer the case, and we can simply update the version of the components repo we can test against.
Interestingly enough, our rollup bundle optimization pipeline did not work properly before 1b827b0. Unused declarations were not elided because build optimizer did not consider the Angular packages as side-effect free. Build optimizer has a hard-coded list of Angular packages that are considered side-effect free. Though this one did not match in the old version of the rollup bundle rule, as internal sources were resolved through their resolved bazel-out paths. Hence build optimizer could not detect the known Angular framework packages. Now though, since we leverage the Bazel-idiomatic `@bazel/rollup` implementation, sources are resolved through linked `node_modules`, and build optimizer is able to properly detect files as side-effect free.
Leverage the caretaker note label configuration in ng-dev's merge tooling to prompt the caretaker for confirmation when a PR has the `PR action: merge-assistance` label. This should help to surface for the caretaker, PRs which may need additional steps taken, announcement messaging, etc.
This comment has been minimized.
This comment has been minimized.
devversion
removed
the
action: cleanup
|
lol that was unexpected🤦♂️ I was hoping it would just refresh the failing Github status after the manual label override. |
|
@googlebot I consent. |
atscott
approved these changes
Jul 9, 2020
|
@googlebot I consent. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
kyliau
approved these changes
Jul 10, 2020
pkozlowski-opensource
approved these changes
Jul 10, 2020
alxhub
approved these changes
Jul 10, 2020
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
…7595) (#37778) Adds support for a caretaker note label to the merge script. Whenever a configured label is applied, the merge script will not merge automatically, but instead prompt first in order to ensure that the caretaker paid attention to the manual caretaker note on the PR. This helps if a PR needs special attention. PR Close #37595 PR Close #37778
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
…ils (#37778) We added a new dependency on `fs-extra` to the dev-infra package. We can remove this dependency and replace it with `shelljs` that is extensively used in other places already. The motiviation is that we can reduce dependencies needed for for consumption of the shared dev-infra package. PR Close #37778
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
Cleans up the dependencies used in the shared dev-infra package configuration. With the recent benchmarking utilities that have been added, a lot of peer dependencies have been added. We decided that we don't want to list every used dependencies as peer dependency as that could result in unnecessary churn/noise for consumers of the dev-infra package. Additionally, not all parts of the dev-infra package are necessarily used. Due to this, we want to apply the following rules for the package dependencies: 1. If a dependency is only used in a shipped Bazel macro/rule that can be optionally consumed, omit it from `package.json`. Bazel reports the missing dependency on its own, so we want to avoid adding it to the package json file. 2. Otherwise, if the dependency is large and commonly used (like buildifier), add it to the `peerDependencies`. If not, add it to the dependencies that are always brought in. We consider it as acceptable to bring in a few small dependencies that might not be used or not. Making all of those option would complicate the use of the dev-infra package. ds PR Close #37778
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
…bClient (#37778) GitClient now uses GithubClient for github API interactions. GithubClient is a class which extends Octokit and provides a member which allows for GraphQL requests against the Github GraphQL api, as well as providing convenience methods for common/repeated Github API requests. PR Close #37778
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
…output (#37778) As of Angular Package Format v10, we no longer ship a `fesm5` and `fesm5` output in packages. We made this change to the `ng_package` rule but intentionally did not clean up related build actions. This follow-up commit cleans this up by: * No longer building fesm5 bundles, or providing esm2015 output. * No longer requesting and building a third flavor for ESM5. We can use TSC to downlevel ES2015 sources/prodmode output similarly to how it is done in `ng-packagr`. The third output flavor (ESM5) resulted in a build slow-down as we required a full recompilation of sources. Now, we only have a single compilation for prodmode output, and then downlevel it on-demand to ES5 for the UMD bundles. Here is timing for building the release packages in `angular/angular` before this change, and afterwards: * Before: 462.157s = ~7.7min * After: 339.703s = ~5.6min This signifies a time reduction by 27% when running `./scripts/build/build-packages-dist.sh`. PR Close #37778
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
Adds the `LinkablePackageInfo` to the `ng_module` rule. This allows the linker to properly link `ng_module` targets in Node runtime actions. Currently this does not work properly and packages like `@angular/core` are not linked, so we cannot rely on the linker. https://github.com/bazelbuild/rules_nodejs/blob/9a5de3728b05bf1647bbb87ad99f54e626604705/internal/linker/link_node_modules.bzl#L144-L146. PR Close #37778
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
…llup` (#37778) Refactors the `ng_rollup_bundle` rule to a macro that relies on the `@bazel/rollup` package. This means that the rule no longer deals with custom ESM5 flavour output, but rather only builds prodmode ES2015 output. This matches the common build output in Angular projects, and optimizations done in CLI where ES2015 is the default optimization input. The motiviation for this change is: * Not duplicating rollup Bazel rules. Instead leveraging the official rollup rule. * Not dealing with a third TS output flavor in Bazel.The ESM5 flavour has the potential of slowing down local development (as it requires compilation replaying) * Updating the rule to be aligned with current CLI optimizations. This also _fixes_ a bug that surfaced in the old rollup bundle rule. Code that is unused, is not removed properly. The new rule fixes this by setting the `toplevel` flag. This instructs terser to remove unused definitions at top-level. This matches the optimization applied in CLI projects. Notably the CLI doesn't need this flag, as code is always wrapped by Webpack. Hence, the unused code eliding runs by default. PR Close #37778
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
It looks like there is a leftover golden in the `ng_package` tests that is no longer used anywhere and does not reflect the latest Angular Package Format v10 changes. We should be able to remove it to keep our codebase healthy. PR Close #37778
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
…37778) The language-service package currently sets the `module` `package.json` property and refers to a folder called `fesm5`. The language-service though does not build with `ng_package` so this folder never existed. Now with APF v10, ng package would not generate this folder either. We should just remove the property as the primary entry-point is the UMD bundle resolved through `main`. There is no module flavour exposed to the NPM package as `pkg_npm` uses the named AMD module devmode output that doesn't work for `module`. PR Close #37778
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
Updates to the latest commit of the `angular/components` repository. We need to do this because we removed the `esm5.bzl` output flavour aspect, but an old version of the components repo relied on this file to exist. This is no longer the case, and we can simply update the version of the components repo we can test against. PR Close #37778
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
Interestingly enough, our rollup bundle optimization pipeline did not work properly before 1b827b0. Unused declarations were not elided because build optimizer did not consider the Angular packages as side-effect free. Build optimizer has a hard-coded list of Angular packages that are considered side-effect free. Though this one did not match in the old version of the rollup bundle rule, as internal sources were resolved through their resolved bazel-out paths. Hence build optimizer could not detect the known Angular framework packages. Now though, since we leverage the Bazel-idiomatic `@bazel/rollup` implementation, sources are resolved through linked `node_modules`, and build optimizer is able to properly detect files as side-effect free. PR Close #37778
atscott
pushed a commit
that referenced
this pull request
Jul 10, 2020
Leverage the caretaker note label configuration in ng-dev's merge tooling to prompt the caretaker for confirmation when a PR has the `PR action: merge-assistance` label. This should help to surface for the caretaker, PRs which may need additional steps taken, announcement messaging, etc. PR Close #37778
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
After this PR, the dev-infra related files in master and patch should be identical.
See individual commits.