Skip to content

feat(core): ship partial compilation output in NPM packages#43431

Closed
devversion wants to merge 74 commits into
angular:masterfrom
devversion:feat/apf-v13-2
Closed

feat(core): ship partial compilation output in NPM packages#43431
devversion wants to merge 74 commits into
angular:masterfrom
devversion:feat/apf-v13-2

Conversation

@devversion

Copy link
Copy Markdown
Member

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

@google-cla google-cla Bot added the cla: yes label Sep 12, 2021
@devversion devversion force-pushed the feat/apf-v13-2 branch 7 times, most recently from 6d663ce to c1adebb Compare September 14, 2021 14:39
@devversion devversion added area: bazel Issues related to the published `@angular/bazel` build rules area: build & ci Related the build and CI infrastructure of the project state: WIP labels Sep 14, 2021
@ngbot ngbot Bot added this to the Backlog milestone Sep 14, 2021
@devversion devversion added the target: major This PR is targeted for the next major release label Sep 14, 2021
@devversion devversion force-pushed the feat/apf-v13-2 branch 17 times, most recently from d3ac365 to dcee15f Compare September 20, 2021 20:18
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).
@devversion

Copy link
Copy Markdown
Member Author

@dylhunn Yeah, just had to push a last time, but this should be ready from my side (assuming CI passes now)

@dylhunn

dylhunn commented Oct 1, 2021

Copy link
Copy Markdown
Contributor

@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!

@IgorMinar

Copy link
Copy Markdown
Contributor

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:

image

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. ❤️ 🚀

@angular-automatic-lock-bot

Copy link
Copy Markdown

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker action: review The PR is still awaiting reviews from at least one requested reviewer area: bazel Issues related to the published `@angular/bazel` build rules area: build & ci Related the build and CI infrastructure of the project cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants