Basis for supporting ESM jasmine node tests #48418
Merged
Conversation
This file contains 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
… 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.
angular-robot
bot
added
the
area: build & ci
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
force-pushed
the
esm-first-migration
branch
2 times, most recently
from
f15ae38
to
f2bb7ac
Compare
devversion
added
PullApprove: disable
target: feature
devversion
added
the
action: review
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.
The underlyijng nodejs tests of the rule now also use the ESM output, so normal `.js` artifacts are not even available there.
…bzl` We introduced a loader that supports ESM with Bazel. This loader can only be enabled as part of our `nodejs_binary` defaults.bzl test. This works fine, but there might be other binaries/tests e.g. from `@angular/build-tooling` that should be able to use ESM & might need to for importing the `compiler-cli`. We move the logic for installing the patch into a `rules_nodejs` patch. There is an existing one that is just updated to "enable ESM support".
For every `ts_library` target we expose a shorthand that grants access to the JS files because `DefaultInfo` of a ts library only exposes the `.d.ts` files. We rename this away from `es2015` since in practice it's a much higher target these days. Additionally we no longer use the devmode output but rather use the prodmode output which has the explicit `.mjs` output- compatible with ESM.
Note: `--require` does not work for ESM. `--import` does not exist in the current Node versions. Started being available in NodeJS v19. A custom entry-point script, already supported by dev-infra, simplifies the whole logic and solves the ESM case.
The prodmode compilation pipeline -that we intend to use more heavily now given it emitting files with the `.mjs` extension- exposes an additional tsickle closure externs file. This file is empty most of the time anyway since tsickle is not wired up. We remove this generation as otherwise convenient `$(location` of such ts library targets break because there always is more than 1 file.
devversion
force-pushed
the
esm-first-migration
branch
from
f2bb7ac
to
c3fd921
Compare
devversion
added
action: merge
|
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. |
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.
See individual commits