Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Final ESM changes #48521

Closed
wants to merge 71 commits into from
Closed

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.
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ngular#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 angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ngular#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 angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ar#48521)

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 angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
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 angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
)

* 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 angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ngular#48521)

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

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ular#48521)

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

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ar#48521)

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 angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
)

* Uses the `.mjs` ESM output for testing/checking circular dependencies.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
* Adjusts tests to no longer rely on CommonJS features. Switches them to
  ESM
* Updates test initialization files to not double-initialize Jasmine now
  that bootstrap files are loaded after Jasmine. The `jasmine.boot`
  setup was hacky from `rules_nodejs` and will break in the future
  regardless if we e.g. use `rules_js` with actual unmodified `jasmine`.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ngular#48521)

ESBuild relies on the linker and we currently set up the ESM loader,
along with accidentally enabling the patched resolution loader. This
didn't cause any problems in sandbox, but outside of sandbox incorrect
ESBuild versions may be discovered because the loader looks at the
top-level `npm/` node modules before looking relative to e.g.
`@bazel/esbuild`

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…dules (angular#48521)

Even with patched resolution, we should always attempt the next/builtin
NodeJS resolution first. It may find a module if there `node_modules`
relative to the context file. This would be more correct than looking
for a module always at the Bazel `npm` repository `node_modules` folder.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…8521)

Since the Bazel setup in this repo will now always use ESM,
the tooling scripts/binaries in AIO need to be switched to ESM
too. Most of the scripts are already ESM, but a few had to be converted.

Note that the Dgeni generation does not use ESM because it's unaffected
and the Dgeni CLI is used. In the future we could also update the Dgeni
setup to ESM but there is no need currently.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…de:` imports (angular#48521)

* Updates build-tooling to benefit from the latest `spec_bundle`
  improvements.
* Updates the ESM extension loader to not attempt adding extensions to
  builtin `node:` specifiers. This seems to be disallowed and cannot be
  handled gracefully (the attempts are part of a try/catch).

```

Use --sandbox_debug to see verbose messages from the sandbox
Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: node:fs.mjs
    at new NodeError (node:internal/errors:371:5)
    at ESMLoader.builtinStrategy (node:internal/modules/esm/translators:276:11)
    at ESMLoader.moduleProvider (node:internal/modules/esm/loader:236:14)
```

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…8521)

Since the `defaults.bzl` repo-wide macros are now supporting ESM,
the special spec-bundle logic from `devtools` can be removed.

Also the esbuild configurations need to be updated to account
for the recent dev-infra build-tooling changes. Also properly
now ensures that `aysnc/await` is downleveled for ZoneJS compatibility.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ngular#48521)

* Switches all examples to use dev-infra's canonical ESM-compatible
  `http_server`.
* Uses ESBuild for bundling ESM into a single file, compared to having
  to load hundreds of individual ESM files in the browser (potential
  source of flakiness & slowness).

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…gular#48521)

* The Karma Bazel Saucelabs binary needs to use `.mjs` as everything in
  the repo w/Bazel is supposed to be ESM.
* The symbol extractor test is updated to no longer use CommonJS
  features like `require.resolve`.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
angular#48521)

The Angular Language Service package is now tested & compiled using ESM.
Previously it was a mismatch of CommonJS and ESM.

Still the language-service ESM output is transformed into a single UMD
bundle to allow for module resolution to be overriden. See
`bundles/BUILD`.

This is kept as is. To fully ship ESM (language-service is an exception
here), we need to:

 * Update all code to no longer reference typescript via import. Instead
   typescript needs to be passed around so that the extension can
   control the version
 * The VSCode extension/ the TS server needs to be able to load ESM. It
   looks like the server supports ESM global plugins, but the extension
   might not yet.

This is out of scope for the dev-infra effort as it requires more
insight into VSCode & the extension system & the TS language server.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
* Updates circular dependency tests to use the `.mjs` outputs
* Switches away from CommonJS specific `require` calls.
* Simplifies the test helper logic since all browsers/NodeJS versions
  support `URL` as a global.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…SM (angular#48521)

* Switches away from the ESM-incompatible & unmaintained `ts_devserver`
  to `http_server`. This is the canonical server maintained by dev-infra
* Switches tests away from CommonJS specific logic. e.g. require.resolve
* Adjusts tests to work with Protractor spec bundling (Protractor does
  not support ESM execution, but we want to take ESM-written specs)
* Reworks playground and benchmarks to use `app_bundle` and `esbuild`
  instead of loading hundreds of files individually. This also makes
  tests more stable and more aligned with real applications.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…lar#48521)

* The benchmark macro should also use devmode ESM 2020. No CommonJS
* The benchmark macro should always add `benchpress` as runtime
  dependency because it is loaded asynchronously.
* The protractor `nodejs_binary` should use our ESM-interop binary
so that ESM resolution works (e.g. when `await import(benchpress)` from
the driver utilities is invoked).

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
After discussion initiated in the framework team (by kkostadinov),
the team has decided to not keeping the `components-repo-unit-tests`
job. This commit removes it.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ngular#48521)

ZoneJS is no longer loaded as an UMD, but instead is included as part
of the browser init entry-point. This means that ZoneJS is bundled and
the ESBuild logic needs to be adjusted for that.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…48521)

* Switches all remaining targets (even if not tested and failing as per
  build) away from `ts_devserver` to the canonical `http_server` from
  dev-infra.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
This is basically a pre-step for combining devmode and prodmode into a
single compilation. We are already achieving this now, and can claim
with confidence that we reduced possible actions by half. This is
especially important now that prodmode is used more often, but rules
potentially still using the devmode ESM sources. We can avoid double
compilations (which existed before the whole ESM migration too!).

We will measure this more when we have more concrete documentation
of the changes & a better planning document.

Changes:

  * ts_library will no longer generate devmode `d.ts`. Definitions are
    generated as part of prodmode. That way only prodmode can be exposed
    via providers.
  * applied the same to `ng_module`.
  * updates migrations to bundle because *everything* using `ts_library`
    is now ESM. This is actually also useful in the future if
    schematics rely on e.g. the compiler.
  * updates schematics for localize to also bundle. similar reason as
    above.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
This will help making it easier to update patches when `node_modules`
are cached. Note that most of the time this shouldn't be necessary
as we only cache the Yarn `.cache` for the non-windows jobs. Windows
job caches `node_modules` directly- so could be affected and needs
a cache reset/wipe occasionally when patches change (even though it's
rarely). Long-term we can remove this when the esm patches are no longer
needed.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ts.bzl` (angular#48521)

Fixes that we temporarily broke the Bazel npm package artifact as
part of the ESM work. This commit adjusts it and also makes the
artifact subsitutions more maintainable.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
)

These files are not used and also should not be synced into g3.

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…e insight (angular#48521)

The way the benchmarking code is used and wired up in g3 is rather
magical. This change makes it easier to sync into g3 by using
conditional blocks, and also consistnetly using a single bundle name
(like it was before— we regressed here as part of the ESM initial
changes- but now with clear comments, it's more future-proof..)

PR Close angular#48521
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
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