feat(bazel): speed up dev-turnaround by bundling types only when packaging #45405

devversion · 2022-03-21T14:23:54Z

Speeds up the dev-turnaround by only bundling types when packaging. Currently
bundling occurs for all the ng_module targets in devmode.

This has various positive benefits:

Avoidance of this rather slower operation in development
Makes APF-built packages also handle types for ts_library targets consistently. Simplifies the packager..
Allows us to ensure APF entry-points have d.ts always bundled (working with ESM
module resolution in TypeScript -- currently experimental -- v13 is not compatible with "moduleResolution": "node12" of Typescript 4.5 #43886)
Allows us to remove the secondary package.json files from APF (maybe APF v14? - seems
low-impact). This would clean-up the APF even more and fix resolution issues (like in Vite -- Secondary entry-point package.json files in APF require mainFields resolution (in Vitest) #45326)

…ion for finding entries Instead of looking for nested `package.json` files which currently only exist for workaround-reasons in APF v13, we should consult the package NodeJS exports. This will help with: angular/angular#45405.

dgp1130

Thanks @devversion, lots of great stuff here! The thorough commit messages are really helpful.

I'm onboard with the overall design, mainly just missing tests. Most of the comments here are nitpicks and questions about various pieces.

packages/bazel/src/ng_package/ng_package.bzl

packages/bazel/src/types_bundle/index.bzl

packages/compiler/src/util.ts

packages/core/src/util/global.ts

packages/bazel/src/types_bundle/BUILD.bazel

packages/bazel/src/types_bundle/index.bzl

devversion · 2022-04-14T19:59:50Z

Thank you! Addressed your feedback @dgp1130. Please have another look when you get a chance (also @josephperrott since I saw you around here 😛)

dgp1130

Looks great! Just a couple more minor things. Tests for types_bundle TS code still seem to be missing and I think the types.to_list() is still there, but otherwise pretty much ready to go.

packages/bazel/src/types_bundle/index.bzl

packages/compiler/src/util.ts

packages/core/src/util/global.ts

packages/bazel/test/types_bundle/BUILD.bazel

packages/bazel/test/types_bundle/index.ts

packages/bazel/test/types_bundle/BUILD.bazel

devversion · 2022-04-15T08:12:12Z

@dgp1130 Addressed your recent comments. Thank you! Regarding your review message:

The to_list extraction still exists, but no longer considers transitive dependencies (like you suggested).
I was thinking a golden test is sufficient for the types bundling. I think this covers the TS logic sufficiently as well.

devversion · 2022-04-21T07:45:18Z

(Disabling the single pending review for a one-line BUILD file change in packages/forms).

devversion · 2022-04-21T12:25:11Z

Caretaker note: This is the first presubmit I ran for a PR, so please double-check if this is right. I ran a TGP with train a few days ago, and now after rebasing a normal TAP presubmit. After deflaking I forced-green and attached the test/ link.

Additionally, before syncing this I think this CL should be incorporated: cl/443330513

atscott · 2022-04-21T18:09:41Z

This PR was merged into the repository by commit 612d6e0.

…#45405) Adds a little golden test for the new `types_bundle` rule that ensures the rule works at a general level. This rule will be useful for non-APF ESM packages like the Angular compiler-cli (for which we also want to bundle types to make them compatible with TypeScripts ESM type resolution) PR Close #45405

The platform-server init entry-point imported code from another entry-point using a relative import. This resulted in the code to be bundled into the `init` entry-point as well. This has no breaking impact but resulted in a little code duplication that we should clean up. PR Close #45405

…45405) This commit addresses two issues: * The init entry-point currenly access code from another entry-point using relative imports, resulting in code to be duplicated. * The init types are now bundled as part of the ng_package APF rule. There is an API extractor bundling issue with global module augmentations. API extractor does not properly handle module augmentation. We need to disable dts bundling for this entry-point to ensure `$localize` remains globally accessible for users. This is an option in the `ng_package` rule. Note that this worked before because `localize/init` was a `ts_library` that did not have its types bundled. As part of this change, the `MessageId` and `TargetMessage` exports are also made public. The localize exported functions rely on these types but they were not exported. Related to types, an exception is added for three private exports from the primary entry-point so that they will show up in the API golden. These private exports are re-exposed publicly in the init entry-point but no golden would capture them due to the private symbol prefix. One might wonder why the symbols are not guarded in the init golden. The reason is that goldens never inline signatures from cross-entry-points/packages to avoid duplication. Lastly, the i18n integration test golden had to be updated because the polyfills bundle increased slightly. After thorough and time-consuming investigation, this mostly happens due to different mangle identifies being used (the input code changed --> so the mangling determinism) Size before this change: ``` SUCCESS: Commit undefined uncompressed runtime did NOT cross size threshold of 500 bytes or >1% (expected: 929, actual: 926). SUCCESS: Commit undefined uncompressed main did NOT cross size threshold of 500 bytes or >1% (expected: 124544, actual: 124660). SUCCESS: Commit undefined uncompressed polyfills did NOT cross size threshold of 500 bytes or >1% (expected: 34530, actual: 34641). ``` After: ``` SUCCESS: Commit undefined uncompressed runtime did NOT cross size threshold of 500 bytes or >1% (expected: 929, actual: 926). SUCCESS: Commit undefined uncompressed main did NOT cross size threshold of 500 bytes or >1% (expected: 124544, actual: 124650). FAIL: Commit undefined uncompressed polyfills exceeded expected size by 500 bytes or >1% (expected: 34530, actual: 35252). ``` Inspecting/comparing without mangling shows that the new changes would actually result in a bundle reduction (potentially visible with gzip/brotli): ``` ➜ Desktop stat -f%z master-nomangle.js 101357 ➜ Desktop stat -f%z with-changes-nomangle.js 101226 ``` PR Close #45405

…ble (#45405) The localize primary entry-point (used at runtime in application code) indirectly loads from the compiler package for computing message ids. The compiler package has a couple of constants which cannot be DCE-ded/ tree-shaken due to side-effect reliance that is detected by Terser. We fix these constants to be three-shakable. Note that another issue technically would be that the compiler package has a side-effect call for `publishFacade` (for JIT), but that invocation is marked as pure by the Angular CLI babel optimization pipeline. So this results is no unused code currently but is risky and should be addressed in the future. PR Close #45405

…` field (#45405) The SystemJS examples were using an outdated version of rollup that did not support export fields. Now with the recent changes where we removed secondary package.json files, the rather old/somewhat outdated SystemJS examples failed bundling since exports were not considered. PR Close #45405

…45405) The main bundle fell below the 500b threshold. Likely because the global constant was fixed and is now tree-shakeable. The actual diff in the commit is a little confusing since it makes it seem that polyfills increased as part of this commit. This is not the case but just a side-effect of us accumulating various changes which are not reflected on a per-commit basis in the size golden. The actual sizes in master were: ``` SUCCESS: Commit undefined uncompressed runtime did NOT cross size threshold of 500 bytes or >1% (expected: 1105, actual: 1102). SUCCESS: Commit undefined uncompressed polyfills did NOT cross size threshold of 500 bytes or >1% (expected: 33846, actual: 33957). SUCCESS: Commit undefined uncompressed main did NOT cross size threshold of 500 bytes or >1% (expected: 132392, actual: 131893). ``` Now with this change: ``` SUCCESS: Commit undefined uncompressed runtime did NOT cross size threshold of 500 bytes or >1% (expected: 1105, actual: 1102). SUCCESS: Commit undefined uncompressed polyfills did NOT cross size threshold of 500 bytes or >1% (expected: 33846, actual: 33957). FAIL: Commit undefined uncompressed main fell below expected size by 500 bytes or >1% (expected: 132392, actual: 131883). ``` PR Close #45405

This was accidentally broken in angular#45405.

…45778) This was accidentally broken in #45405. PR Close #45778

In PR angular#45405, the Angular Package Format (APF) was update so that secondary entry-points (such as `@angular/common/http`) do not have their onw `package.json` file, as they used to. Instead, the paths to their various formats and types are exposed via the primary `package.json` file's `exports` property. As an example, see the v13 [@angular/common/http/package.json][1] and compare it with the v14 [@angular/common/package.json > exports][2]. Previously, `ngcc` was not able to analyze such v14+ entry-points and would instead error as it considered such entry-points missing. This commit addresses this by detecting this situation and synthesizing a `package.json` for the secondary entry-points based on the `exports` property of the primary `package.json` file. This data is only used by `ngcc` in order to determine that the entry-point does not need further processing, since it is already in Ivy format. [1]: https://unpkg.com/browse/@angular/common@13.3.5/http/package.json [2]: https://unpkg.com/browse/@angular/common@14.0.0-next.15/package.json

In PR angular#45405, the Angular Package Format (APF) was updated so that secondary entry-points (such as `@angular/common/http`) do not have their own `package.json` file, as they used to. Instead, the paths to their various formats and types are exposed via the primary `package.json` file's `exports` property. As an example, see the v13 [@angular/common/http/package.json][1] and compare it with the v14 [@angular/common/package.json > exports][2]. Previously, `ngcc` was not able to analyze such v14+ entry-points and would instead error as it considered such entry-points missing. This commit addresses the issue by detecting this situation and synthesizing a `package.json` file for the secondary entry-points based on the `exports` property of the primary `package.json` file. This data is only used by `ngcc` in order to determine that the entry-point does not need further processing, since it is already in Ivy format. [1]: https://unpkg.com/browse/@angular/common@13.3.5/http/package.json [2]: https://unpkg.com/browse/@angular/common@14.0.0-next.15/package.json

In PR #45405, the Angular Package Format (APF) was updated so that secondary entry-points (such as `@angular/common/http`) do not have their own `package.json` file, as they used to. Instead, the paths to their various formats and types are exposed via the primary `package.json` file's `exports` property. As an example, see the v13 [@angular/common/http/package.json][1] and compare it with the v14 [@angular/common/package.json > exports][2]. Previously, `ngcc` was not able to analyze such v14+ entry-points and would instead error as it considered such entry-points missing. This commit addresses the issue by detecting this situation and synthesizing a `package.json` file for the secondary entry-points based on the `exports` property of the primary `package.json` file. This data is only used by `ngcc` in order to determine that the entry-point does not need further processing, since it is already in Ivy format. [1]: https://unpkg.com/browse/@angular/common@13.3.5/http/package.json [2]: https://unpkg.com/browse/@angular/common@14.0.0-next.15/package.json PR Close #45833

angular-automatic-lock-bot · 2022-05-22T16:06:00Z

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 force-pushed the types-bundling branch from b41cd17 to 87200c9 Compare March 21, 2022 14:54

devversion force-pushed the types-bundling branch 4 times, most recently from 79ac211 to 723128a Compare March 21, 2022 19:33

dylhunn added the area: bazel Issues related to the published `@angular/bazel` build rules label Mar 24, 2022

ngbot bot added this to the Backlog milestone Mar 24, 2022

devversion force-pushed the types-bundling branch 5 times, most recently from d0e31dd to ed601e0 Compare April 13, 2022 08:18

devversion added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Apr 13, 2022

devversion requested review from josephperrott and alxhub April 13, 2022 13:41

devversion marked this pull request as ready for review April 13, 2022 13:41

pullapprove bot requested a review from AndrewKushnir April 13, 2022 13:41

devversion requested a review from dgp1130 April 13, 2022 17:01

dgp1130 reviewed Apr 13, 2022

View reviewed changes

devversion force-pushed the types-bundling branch from 2e20deb to bdbe5bd Compare April 14, 2022 19:57

devversion requested a review from dgp1130 April 14, 2022 19:59

devversion force-pushed the types-bundling branch from bdbe5bd to 19b2a43 Compare April 14, 2022 20:43

dgp1130 reviewed Apr 15, 2022

View reviewed changes

devversion force-pushed the types-bundling branch from 19b2a43 to 4da05a9 Compare April 15, 2022 08:08

devversion added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Apr 21, 2022

atscott closed this in 68597bb Apr 21, 2022

devversion mentioned this pull request Apr 22, 2022

docs: update angular package format guide to reflect recent changes #45725

Closed

gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 27, 2022

docs(upgrade): move import back into docregion in rollup-config.js

de528b4

This was accidentally broken in angular#45405.

gkalpak mentioned this pull request Apr 27, 2022

docs(upgrade): move import back into docregion in rollup-config.js #45778

Closed

devversion mentioned this pull request Apr 27, 2022

v13 is not compatible with "moduleResolution": "node12" of Typescript 4.5 #43886

Open

atscott pushed a commit that referenced this pull request Apr 27, 2022

docs(upgrade): move import back into docregion in rollup-config.js (#…

71f64f2

…45778) This was accidentally broken in #45405. PR Close #45778

gkalpak mentioned this pull request Apr 30, 2022

fix(ngcc): cope with packages following APF v14+ #45833

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bazel): speed up dev-turnaround by bundling types only when packaging #45405

feat(bazel): speed up dev-turnaround by bundling types only when packaging #45405

devversion commented Mar 21, 2022 •

edited

dgp1130 left a comment

devversion commented Apr 14, 2022

dgp1130 left a comment

devversion commented Apr 15, 2022

devversion commented Apr 21, 2022

devversion commented Apr 21, 2022

atscott commented Apr 21, 2022

angular-automatic-lock-bot bot commented May 22, 2022

feat(bazel): speed up dev-turnaround by bundling types only when packaging #45405

feat(bazel): speed up dev-turnaround by bundling types only when packaging #45405

Conversation

devversion commented Mar 21, 2022 • edited

dgp1130 left a comment

Choose a reason for hiding this comment

devversion commented Apr 14, 2022

dgp1130 left a comment

Choose a reason for hiding this comment

devversion commented Apr 15, 2022

devversion commented Apr 21, 2022

devversion commented Apr 21, 2022

atscott commented Apr 21, 2022

angular-automatic-lock-bot bot commented May 22, 2022

devversion commented Mar 21, 2022 •

edited