Skip to content

Final ESM changes#48521

Closed
devversion wants to merge 71 commits intoangular:mainfrom
devversion:esm-first-migration
Closed

Final ESM changes#48521
devversion wants to merge 71 commits intoangular:mainfrom
devversion:esm-first-migration

Conversation

@devversion
Copy link
Member

@devversion devversion commented Dec 16, 2022

Overall goal:

  • Currently our Bazel setup still produces CommonJS output. This output is mostly used by tests. Even though we ship ESM to our users through NPM, we test CommonJS here in the repository.
  • Given that we test CommonJS, but ship ESM- our code needs to be able to work with both module systems. e.g. using require in a safe way, but also not being able to use import.meta.url (since it's unknown syntax for CJS).
    • Removing CommonJS here- allows us to simplify our code significantly
    • An easier mental model of "ESM only"
    • We test more closely what we actually ship more
  • No longer relying on the ts_library/ng_moduledevmode/prodmode constructs allows us to migrate to e.g.rules_jsin the future (a better & maintained variant ofrules_nodejs`).
    • Rules NodeJS originated out of g3 and shows its ESM incompatibilities.
    • That's the reason we need to patch/trick the current rules into actually supporting ESM at all..

See individual commits

Currently the devmode output for `ng_module` and `ts_library` is
using ES5 CommonJS UMD. To bring it in sync with prodmode and
to start with our long-term migration to full ESM- the devmode
is updated to to ES2020 ES modules too.

This will require more tricks to make devmod work with the bazel
setup and also tests may need to be refactored given them relying
on ES5 CJS features, like for `spyOn` jasmine patching etc.
This is in prearation for having a proper diff when this loader
is adjusted to support more situations than just simple external
node modules. See next commit.

Also the file is formatted to make the diff less verbose later.
The file was never formatted correctly and we don't lint `.mjs` files.
@angular-robot angular-robot bot added the area: build & ci Related the build and CI infrastructure of the project label Dec 16, 2022
@ngbot ngbot bot added this to the Backlog milestone Dec 16, 2022
@devversion
Copy link
Member Author

Caretaker note: When merging, patch https://critique.corp.google.com/cl/496086044 before Copybara

@devversion 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 Dec 17, 2022
@devversion devversion added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Dec 17, 2022
@devversion devversion marked this pull request as ready for review December 18, 2022 09:45
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@josephperrott
Copy link
Member

Looks like there is an error in some of the commit messages syntax

@devversion
Copy link
Member Author

Yeah, now that I have your approval, I will re-target this PR to main (from esm-stage-1) and will now fix up the commit messages when all commits are part of it.

@devversion devversion changed the base branch from esm-stage-1 to main December 19, 2022 14:41
Replaces the existing ESM loader for dealing with external module
imports. This loader was introduced by Aspect for AIO `.mjs` scripts.

The loader will be used as foundation for a more extensive loader
that also properly handles first-party packages.

Additionally another loader is added, all packed as a single
loader because our current NodeJS version only supports a single
loader per node invocation. So we implement chaining ourselves.

The new loader will attempt rewriting `.js` extensions to `.mjs`,
also it will add `.mjs` if not already done. This is necessary
in the transition phase because we don't/cannot use explicit `.mts`
extensions and also we don't specify extensions in imports yet.

Long-term we would likely use `.mts` and explicit import extensions,
but it's not yet clear how we would sync this into g3 too.
Since this repo will now be strict ESM, and Angular Compiler packages
on NPM are also ESM-only, we can rework `ngc-wrapped` to remove
the CJS/ESM interop and we make it strict ESM too.
The `generate-locales-tool` now needs to run as ESM because we changed
the `ts_library` rule. This commit accounts for the ESM syntax but
removing CJS code parts like `require.main === module`.

Also imports using `require` need to be changed to their ESM
equivalents.
… use ESM

The `__ESM_IMPORT_META_URL__` trick was used because we used both ESM
and CommonJS in this repo. It was an interop needed because
`import.meta.url` syntax couldn't be used as it woud have caused syntax
errors.

We still need to keep the CommonJS/ESM interop in the compiler-cli
because g3 relies on the compiler and uses CommonJS. This affects very
few places, just in the compiler- so this is acceptable.
…y/test`

The Bazel NodeJS rules will always use the `.js` files as entry-points.
Since we only rely on the `.mjs` output going-forward, we need to teach
`nodejs_binary` and `nodejs_test` to use the `.mjs` extensions if
intended.

Our `defaults.bzl` macros will set `use_esm = True`, but other targets
from e.g. external repositories should keep the original behavior.
The `nodejs_binary` rule already prioritizes the `.mjs` output as
of the recent commits. The `nodejs_test` rule should do the same,
and also set `use_esm = True`
Removes `shelljs` which is a known ESM-problematic dependency.
We don't need it anyway.
We modified the macros of `nodejs_binary/test` to have a rule
in between that requests the `.mjs` output. This works fine but
breaks make variable substitution for `templated_args` because
Bazel requires referenced labels to be part of the explicit `data`.

The rule in between breaks this, so we add a new argument that
can be used for such "template"/"args" data dependencies.

This can be removed when everything is ESM and we don't need
the rule in between.
@devversion devversion force-pushed the esm-first-migration branch 2 times, most recently from 92fb036 to 23ebc61 Compare December 19, 2022 14:58
For the ESM interop patches we expect to have two types
of patches:

* Patches for `node_modules`
* Patches for Bazel repositories.

We move the patches in respective folders to make it very clear
where a patch is used/applied to.
This is necessary for e.g. JSON files from the CLDR data repository.
Otherwise these could not be added to the runfiles of the
binaries/tests.
devversion added a commit that referenced this pull request Dec 19, 2022
The ESM loader when used with patched Bazel module resolution
handled subpaths incorrectly. e.g.
`@bazel/concatjs/tsc_wrapped/internal/index.js` could be incorrectly
resolved to `@bazel/concatjs/index.js`.

This commit fixes the flawed logic. Also we prioritize the ESM attempts
before the original specifier. This is necessary because otherwise
the default Node resolution (using `require.resolve`) may incorrectly
prefer the `index.js` file over a `index.mjs` when a directory is
imported.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
We use `bazel/esbuild` in various places (e.g. for app bundling tests).
These tests rely on the Angular Compiler-CLI itself for e.g. linking
or the Terser configuration. Since everything in this repo is now
strict ESM, the ESBuild configs (which are already ESM-supported)
need to import from `//packages/compiler-cli`. We also need to be
able to leverage our existing ESM Bazel loader for this though as
otherwise resolution would fail.

Long-term we can remove this if everything in the compiler-cli
would use `.mjs` extensions and the import paths would also specify
an explicit extension. See: https://nodejs.org/api/esm.html#mandatory-file-extensions

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
The symbol extractor test tool now runs with ESM too. This commit
makes it ESM compatible.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
Switches all `core/test/bundling` tests to not rely on
CommonJS-specific features like `require.resolve

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
…48521)

Update dev-infra's build-tooling since multiple ESM changes have
landed there. e.g. not relying on `require.main === module` for API
bundling. This will allow us to also execute all dev-infra rules
in ESM because we plan on applying our ESM patching to `ts_library`
(which would also affect build-tooling then).

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
…o JS output (#48521)

If tests are bundled using e.g. esbuild, the `ee` symbols may
be rewritten to `\u0275\u0275`. This breaks assertions that
rely on `Function.toString`. We can avoid this string comparison
and make it more future proof by just comparing the symbols directly.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
Since Karma with Bazel does not support ESM natively, we bundle the
tests using ESBuild into a single AMD file. This not only solves the
ESM issue until we can run browser ESM tests natively (also pending
in the components repo - the esbuild generation follows ESM semantics
but since collapsed we don't rely on the real module system).

A benefit of bundling is also faster and more reliable Karma browser
tests since only a single file needs to be loaded- compared to hundreds
of individual files.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
)

The Angular CLI does not yet support schematics running as ESM. For
this reason we switch the schematics BUILD targets to explicitly
use ESM (as an exception in the repo).

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
Properly imports `jasmine-ajax` instead of attempting to load
some CommonJS bundle as a bootstrap dependency in Karma.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
The circular deps tests should use the `.mjs` output.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
)

We no longer have generated `.js` files as everything generated
is the ESM `.mjs` output. The tests need to be updated to reflect that.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
The bazel tests cannot use CommonJS features like `require` and should
be updated to work with ESM. Additionally some specs are updated to
no longer assert CJS/UMD output from `ng_module`. The rule only emits
ESM now.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
Switches the circular deps test to use the ESM output. The
normal `.js` devmode output is no longer available.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
Since the linker is no longer being tested with CommonJS, we can remove
most of the CJS/ESM interop trickery.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
Refactors ngcc to only rely on ESM features because CJS
is no longer needed & tested.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
Updates compiler-cli & tests to be full ESM compatible. Tests
no longer with CommonJS.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
`platform-browser` tests now run in ESM and with `.mjs` output, so
the build targets and tests need to be updated.

Here we change the `zone_event_unpatched` script to include the
`.init` suffix that will be picked up by `spec_bundle`.

Also some circular dependency tests are updated to refer to the
`.mjs` files.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
* Switches to the canonical dev-infra http server
* Uses the bundle for serving.
* Switches app_bundle to simple `esbuild` since the test relies
  on `ngDevMode` which `app_bundle` elides as optimization.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
Introduces a variant of `jasmine_node_test` that works with async/await
in combination with ZoneJS. This is needed for tests relying on
async/await in combinatin with change detection.

Browser tests are already benefting from ESbuild async-await
downleveling. Node tests would ideally not need this, but until
ZoneJS supports native async/await we need to also process
source files to avoid native async/await syntax.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
…48521)

The ESM js files need to be referenced, and the router tests rely
on `async/await` with change detection- so a special rule is required
that downlevels `async/await` to generators (similar to the Angular CLI)

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
…48521)

There are two build targets which never had all its runtime dependencies
properly specified. This wasn't noticed because there were macros in
`defaults.bzl` that automatically included these deps.

In a follow-up we will clean-up this legacy auto-deps feature in
`defaults.bzl`.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
Protractor does not support ESM, so we need to take all the ESM
output and bundle it into a CommonJS file. This comes at the cost
of not using actual ESM for execution, but the ESBuild bundle follows
strict ESM semantics so we can be sure it's compatible when we have
an ESM-compatible e2e test runner in the future.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
The `packages/localize` package still required some trickery
to support CommonJS because tests in the repo were running as CommonJS.

This commit removes the CommonJS logic in localize and its tests, so
that only ESM is used in production & tests.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
* Uses `.mjs` for circular deps tests
* Replaces `require` with ESM-equivalent. uses an import statement here
  for proper typing, and specify dependency properly.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
…48521)

* Switches circular dependency tests to use the `.mjs` output.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
)

* Uses the `.mjs` output of the upgrade package when checking for
  circular deps.

PR Close #48521
devversion added a commit that referenced this pull request Dec 19, 2022
Tests now always run with ESM 2020, while previously they ran with
ES2015 CommonJS UMD bundles.

Since ZoneJS does not support intercepting native `async/await` syntax,
the forms test needs to use the zone-compatible variant of
`jasmine_node_tests`. This variant downlevels the native `async/await`
syntax to generators that ZoneJS can intercept. All of this is done
using the dev-infra ESBuild `spec_bundle` rule.

PR Close #48521
@angular-automatic-lock-bot
Copy link

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 area: build & ci Related the build and CI infrastructure of the project merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note PullApprove: disable target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants