Closed
Conversation
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.
343e8bd to
b5d4692
Compare
Member
Author
|
Caretaker note: When merging, patch https://critique.corp.google.com/cl/496086044 before Copybara |
6ed7a21 to
fee8bee
Compare
Member
|
Looks like there is an error in some of the commit messages syntax |
Member
Author
|
Yeah, now that I have your approval, I will re-target this PR to |
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.
92fb036 to
23ebc61
Compare
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
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) 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
) * 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
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overall goal:
requirein a safe way, but also not being able to useimport.meta.url(since it's unknown syntax for CJS).ts_library/ng_moduledevmode/prodmode constructs allows us to migrate to e.g.rules_jsin the future (a better & maintained variant ofrules_nodejs`).See individual commits