feat(core): ship partial compilation output in NPM packages#43431
feat(core): ship partial compilation output in NPM packages#43431devversion wants to merge 74 commits into
Conversation
6d663ce to
c1adebb
Compare
d3ac365 to
dcee15f
Compare
As of v13, the package output will be using partial compilation output. This breaks the Bazel setup similar to how it breaks Angular Components. The problem is that `@bazel/concatjs` relies heavily on the UMD files that previously existed in APF, plus it assumed that ngcc pre-processed the files in the `node_modules`. This is no longer the case as there are no UMD files, and the code is not fully-compiled by the Angular compiler.
Removes the remaining usages of dynamic require statements in the package output. Since we declare all shipped packages as strict ESM, we cannot use dynamic require statements anymore. This commit switches these usages to actual `import` statements. Note: Tsickle continues to remain an optional dependency since bundling does not work with its UMD package output. Also tsickle is rarely used by consumers, if at all, so bundling does not really provide any significant value. To continue keeping tsickle optional (since it's still needed by the `annotateForClosureCompiler` option which is also respected in ngtsc), we pass-through a tsickle instance as a parameter to `main`. This allows us to keep the compile functions synchronous without having to refactor the majority of the watch compilation code, and majority of tests for ngc, ngtsc. Consumers (like the `ngc` bin entry-point) can then load tsickle based on their module format. e.g. tsickle can be imported through `require` to keep everything sync, but in ESM, the dynamic import can be used beforehand to pass `tsickle` to the `main` function. We can revisit this in the future but for now this does the trick without exceeding the scope of this commit..
… for ESM compatibility Switches the compiler-cli usage of `__filename` to `import.meta.url` when ESM bundles are generated. Unfortunately we cannot start using only `import.meta` yet as we still build and run all code in Angular in CommonJS module output for devmode tests. This commit also fixes various instances where a jasmine spy was applied on a namespace export that will break with ES module (and the interop for CommonJS output). We fix these spies by using a default import.
…ith ESM Updates the lock file resolution logic in ngcc to work with ESM output. The compiler-cli is now shipped in bundles, so the actual module resolution needs to stay to keep the lock file path consistent regardless of where the lock file code is bundled into. The ngcc integration test needs to be updated though since the `ngcc` entry-point will always reside in the `bundles/` directory now. It has been considered using the top-level `package.json` of the compiler-cli package, but that caused problems in tests down the line because the ngcc tests only have the `@angular/compiler-cli/ngcc/...` targets linked into the node modules. It's not worth changing this and reworking tests if ngcc is going away in the future anyway (+ it has been like that before!).
Given that we ship all of compiler-cli and localize in ESM mode now, we need to use a ESM compatible version of Yargs. The latest version seems ESM compatible but with some small API changes. This commit updates Yargs and updates the command line option code to use the new API.
…rked as busy with ESM Ngcc relies on cluster for distributing work. The master controller sends messages to the workers as soon as the worker becomes `online`. The online event is sent as part of the NodeJS cluster logic itself. This does not work well because technically `online` could emit before the worker started listening (this seems to be case now with ESM as the imports are loaded in a way where `online` emits too early; before the worker actually listens for messages). We fix this by explicitly notifying the master when the worker is ready for retrieving IPC messages/or tasks. This is more safe anyway as it's not clearly specified when `online` emits.
In the current Bazel setup, we run CommonJS as devmode output, and ESM for prodmode output. This means that consumers of the `@angular/bazel` package will end up using the prodmode-built ESM package of the compiler-cli. This commit adds interop logic to be able to load the compiler-cli as strict ESM package. We cannot switch devmode to ESM, as this would require some changes in `rules_nodejs` and potentially the reduction of both output flavors into a single one (which is a future project anyway). This is out of scope for now and inside g3, there is still devmode output as well.
Invalidate the cache for node modules so that the CLI builds from the snapshot repositories are used. Due to a bug in Yarn, and the need of using the `github:<..>` syntax, the old artifacts from previous runs are incorrectly cached. This could have been prevented by using an actual full Git ref URL, but unfortunately that does not work because the CLI devkit snapshot dependencies set snapshot builds as deps as well. The URLs here need to match as otherwise Yarn will conflict. e.g. ``` Pattern ["@angular-devkit/core@github:angular/angular-devkit-core-builds#64b7e2b1d"] is trying to unpack in the same destination "/Users/paul/Library/Caches/Yarn/v6/npm-@angular-devkit-core-13.0.0-next.6/node_modules/@angular-devkit/core" as pattern ["@angular-devkit/core@github:angular/angular-devkit-core-builds#0e7277c63"]. This could result in non-deterministic behavior, skipping. ```
Updates the dynamic-compiler test to be compatible with the APF v13. As of v13, the packages no longer come with metadata.json files and now need to be processed with the babel linker plugin. This commit sets up the linker plugin, and switches away from the deprecated systemjs approach to a simpler rollup code-splitting variant.
Updates the `ng_elements` integration test to work with the APF v13 format by also enabling the linker for the FW packages.
Updates the `hello_world_closure` integration test to use APF v13 in combination with the linker plugin which is needed as running the `ngc` command standalone does not modify the `node_modules` and the FW packages remain partially compiled. A step in between using rollup can create a linker-processed bundle of all FW packages.
Adding suites to the `IGNORED_EXAMPLES` array currently results in an runtime exception because later when ignored examples are printed to stdout, a non-existent variable is referenced back from when there were examples disabled due to the Ivy migration. This commit fixes the logic.
…to chromium update We updated the dev-infra version as part of the v13 package format in order to be able to use the latest `rollup` version (and its plugins). The update of the shared dev-infra package resulted in an update of Chromium to a more recent version. This version of Chromium seems to normalize the `content-type` header differently, so that the AIO `http` example test assertion needs to be updated to work with the new version of chromium. This commits updates the test.
…piler` With the APF v13 package output, deep files can no longer be imported. Since we do not intend to bundle the compiler into the compiler-cli, we need to switch all deep imports to the primary entry-point.
This commit temporarily disables the SystemJS upgrade e2e tests. All of the upgrade e2e tests (except for `phonecat-1-typescript`) rely on UMD bundles. These are no longer available for the v13 Angular package output, so we disable the tests for now. These e2e tests can be re-enabled once we migrated the exampels from UMD bundles to e.g. the CLI, or some custom rollup build. Alternatively it might be even possible to use FESM bundles directly (depending on browser support for the AIO examples; this is something the docs-infra team will have to determine though).
Temporarily disables the Bazel integration test as it will not work with the APF v13 output which is strict ESM. We need to land some logic in `rules_nodejs` first that would allow an ESM variant of `@angular/compiler-cli` to work. Once this happened and there is a new release, we can re-enable the test and make adjustments for v13 APF (i.e. running the linker plugin when creating the rollup bundles).
The package builder script should respect the `BAZEL` environment variable for running Bazel. If not set, it can fallback to bazelisk from the `node_modules`. Respecting this variable allows for users with a global `bazel` binary. This is desirable in some situations, like on Windows, where running Bazel inside of the Yarn environment seems slower than running a global variant. This could appear like that because projects might use different Bazel versions. In some cases, developers would want to use a single (already-warmed-up) instance of Bazel instead of launching different versions using bazelisk. (e.g. when switching a lot between repos like COMP, FW or CLI..) In any case, it doesn't hurt providing this flexibility for advanced use-cases. It's low-effort to maintain and is respected in COMP as well.
…ssing runfile symlinking Currently, some tests in the `compiler-cli/integrationtest` package fail on Windows because there are spec files which are not Bazel-generated. When Bazel runs these tests on Windows, the spec file is resolved to the actual source file (since there is no runfile symlinking/sandboxing). This breaks the execution of the CJS spec file since it resides in th `packages/compiler-cli` source folder which has a `package.json` set to `type: module`. We fix this by adding a `package.json` file for the integration test folder and setting `module` to `commonjs`.
…oint Similar to the other private entry-points we have added for localize, bazel or the migrations, we should expose the tooling code through a dedicated private export. This will make the compiler-cli exports more consistent and it will become easier for the CLI to export necessary code.
Simplifies the `last_segment_name` computation in the integration test Starlark macro we use. The last segment name could be computed in a shorter way and this has come up while being at it (through review; so this commit addresses that).
This commits sets the JS target for all command line tools to NodeJS v12. ESbuild will automatically downlevel the ES2020 features we currently use to make them compatible with NodeJS v12 <-> ES2019. ES2020 is the prodmode output, but we still support Node v12 so there needs to be some downleveling for now. Note: This is a separate commit because initially the target was set to Node v14 to match up with the prodmode Bazel output.
Temporarily disables the ng_update_migrations integration test until angular#43657 lands.
Updates the size goldens for the two AIO production build jobs. Due to the removal of differential loading, the bundle names no longer include the ES version being used, so we remove that suffix. The styles overall decreased. Additionally, the main bundle became noticable smaller, with a little increase in the polyfills. The AIO job not using APF v13 seems larger but this is likely due to the current Angular v13 packages (in the `test_aio`) job still using View Engine APF v12 with the CLI v13 (where some optimizations might not match up anymore).
Updates the size goldens for the integration CLI tests to reflect the new payload with APF v13 and the CLI v13-next.7 Overall, there seems to be some increase in the polyfills and a ~400b increase for the `main` bundles. This seems to because with APF v13, the core package no longer uses the downleveled `Object.assign`, but the spread operator directly. ESbuild will then downlevel the spread to `__spreadProps` which seems to come with more transitively-required helpers that end up contributing to the ~400b increase. The spread is downleveled even for the modern browsers this integration test targets, because it is a trick to wrokaround a performance bug in V8. So the size increase is reasonable given the runtime improvement. More details here: evanw/esbuild#951 (comment).
|
@dylhunn Yeah, just had to push a last time, but this should be ready from my side (assuming CI passes now) |
Great. I'll wait for everything to go green, then merge and sync. I'm a bit scared of missing stuff in the Slack, so pls remove the merge label if that changes! |
|
just for the record keeping: we accidentally merged this a bit earlier than intended - many of us have reviewed the PR but we haven't explicitly approved it because we were waiting to see the CI go green. That did happened before the PR got merged: All in all, this PR is a work of art and I admire @devversion for having the patience and skill to craft such a beautiful (easy to review) and impactful PR. Well done everyone who pitched in with partial fixes, advice, reviews, etc. ❤️ 🚀 |
|
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. |

Implements the Angular Package Format v13 which comes with
partial compilation output and ES2020.