Support / require ES modules #33
Comments
Refs #33. In ESM, we need to do a default import of `yargs` and then call it as a function with arguments passed in. See: https://yargs.js.org/2020/09/07/yargs-16.html. For some reason I can't get `yargs/helpers` to import, but it's not that important.
Refs #33. For some reason we were on a very old version of `node-html-parser` and needed to upgrade it to gain ESM support. This introduced a few breaking changes we needed to be addressed, but nothing too significant.
Refs #33. Default import doesn't work when using true ESM, so we need to import by symbol name.
…, `*.mts`, `*.d.cts`, and `*.d.mts`. Refs #33.
Refs #33. This fixes aspect-build/rules_jasmine#33 and adds ESM support. I've sent a PR in aspect-build/rules_jasmine#41 to apply the fix upstream, but for now a patch will have to do.
Refs #33. The actual version of Jasmine used comes from `jasmine_repositories()`, not `package.json`. `@aspect_rules_jasmine` runs its own `npm install` under the hood to get the specific version of Jasmine. This commit drops the `jasmine` dependency in `package.json`, which is actually unused. It also bumps the `@types/jasmine` dependency to the current latest. Unfortunately `jasmine` and `@types/jasmine` don't directly correlate, so we can't forcibly align the versions there.
Refs #33. This is useful to mock and stub the file system without relying on spying on imports in Jasmine, which doesn't work with ESM. I investigated a few existing "fake file system" dependencies, however I wasn't really happy with how any of them worked or the supply chain implications of using them. I'm hoping to keep file system usage simple enough that it won't grow to be a maintenance headache, but we'll see how things shake out.
Refs #33. This mostly required dropping the Jasmine spy on the `fs` module.
Refs #33. This mostly involves replacing the spy on the `fs` module with usage of a fake file system.
Refs #33. This module's entire purpose was to proxy the underlying Node.js `fs` module in a way which could be spied upon. However, in native ESM we can't spy on even user modules which are imported. We already have `FileSystem` and `FileSystemFake` as a replacement and all tests were successfully migrated to it, so `fs` is no longer needed.
Refs #33. ESM doesn't support spying on imports, so instead we need to reset the map on each test rather than spying on the getter.
Refs #33. This expands the file filter to allow `*.mjs` or `*.cjs` files in the Rollup config, which will be necessary for ESM.
Refs #33. We cannot spy on imported modules in native ESM, and we don't actually need those assertions anyways since we were calling through to a real WebDriver instance and actually invoking commands on a web page. The spies are wholly unnecessary to these tests.
Refs #33. File extensions shouldn't be included as of now and this will break when they change during the switch to native ESM (`*.mjs`).
Refs #33. This updates `@rules_prerender` to use ESM modules everywhere. This makes a few changes throughout the workspace: 1. Renames `*.{ts,js}` -> `*.{mts,mjs}`. 2. Updates imports to include `.mjs` in the file path. 3. Updates any hard-coded CommonJS `require()` or `module.exports` to use ESM syntax. 4. Updates `tsconfig.json` files to use `module: "ES2020"` and `moduleResolution: "node"`. 5. Updates `package.json` files to use `type: "module"`. Most of the changes are fairly mechanical syntax modifications, though there are a couple random edits thrown in as necessary to switch over to ESM. Most of this was generated by running: ``` $ echo {common,examples,packages,tools}/**/*.ts | tr " " "\n" | xargs -I {} bash -c "git mv {} \$(echo {} | sed 's,\\.ts$,.mts,g')" $ sed -i -E "s,^import (.*?) from '(\..*?)';,import \1 from '\2.mjs';,g" {common,examples,packages,tools}/**/*.mts $ sed -i -E "s,^export (.*?) from '(\..*?)';,export \1 from '\2.mjs';,g" {common,examples,packages,tools}/**/*.mts $ sed -i "s,\\.ts\",.mts\",g" {.,common,examples,packages,tools}/**/BUILD.bazel $ sed -i "s,\\.js\",.mjs\",g" {.,common,examples,packages,tools}/**/BUILD.bazel ```
…efault. Refs #33. This was a very large commit which touched basically every TypeScript and BUILD file in the repo in a purely mechanical fashion, so hiding it simplifies history a bit.
|
After a lot of work I was able to switch First, I basically have to name all the files Second, I needed to remove all the places where I used Jasmine spies on imports. It is not possible to mutate an imported object, so spies don't work in that context. I had to update the code to use proper dependency injection. Third, Fourth, I tried to using Fifth, one particular challenge I'm not sure I got right is This is published in |
ES modules are the future and we should support and/or require their use. The main challenge here is getting this to work in the NodeJS toolchain and well enough that it isn't a burden on application developers.
I starting look at this and spent the last few hours hacking on it but have failed miserably to get anything to work. Currently,
rules_nodejsruns Nodev12.13.0by default:This does not support ES modules natively, however it can be enabled with the
--experimental-modulesoption (passed as--node_options=--experimental-modulesastemplated_argsto thenodejs_binary()targets). This is enough to enable the feature, but the tricky part is actually compiling everything into compatible JavaScript.I tried updating
"module": "ES2020"in thetsconfig.json, however it seems thatts_library()just ignores this field and does its own thing. Instead,ts_library()emits anes5_sourcesandes6_sourcesoutput group.es5_sourcesis the default, however I can force it to usees6_sourcesviafilegroup(output_group = "es6_sources")(as documented here). This grabs the*.mjsfiles compiled with ES modules. The problem with this is that it only collects the ES6 outputs of the given target, it does not include transitive dependencies, which is basically useless.I found an
es6_consumerexample which works around this by manually collecting all the transitive sources. With this, all the files are present and use the.mjsextension necessary for Node to treat them as modules.We then have to drop
--bazel_patch_module_resolverso it uses the real ESM loader, but any imports still fail with a message like:My understanding of this is that I'm importing via a
.jsextension is still interpreted as a CommonJS module and attempts to be converted to ESM. Since it is not a CommonJS module, it doesn't actually export anything and fails. The error message is quite misleading.Importing without an extension gives a "Cannot find module" error, which makes me think NodeJS isn't resolving the extension for me. The desirable solution would to import with an
.mjsextension, but both VSCode and TypeScript reject this.It seems that TypeScript does not have any meaningful support of
.mjsas either an input or output file extension atm. This means I'm stuck between Node which wants.mjsand TypeScript which wants no extension or.js. I'm not sure how to resolve this (no pun intended).I tried taking a step back and going at this via
ts_project(), which is a bit simpler build system and closer to the underlyingtsccommand. If we convert everything to usets_project(), could that work better? Short answer, not really. To my knowledge there is no way to maketscoutput an.mjsextension (and TypeScript doesn't support that anyways as mentioned earlier). This means we're stuck with a.jsextension, and the only other way to make Node recognize a file as a Node module is to specific"type": "module"in thepackage.json. That sounds easy, but setting it in the rootpackage.jsonhas no effect. I get the following error:Looking at the generated tree,
package.jsonis included. And even if I explicitly depend onpackage.jsoninnodejs_binary()or acopy_to_bin()version, this error message does not change. Using absolute imports withlink_workspace_root = Truealso has no effect.Looking at the
common/binary.jsfile which throws the error, it is contained inside thepackage.jsonfile at the artifact root. Based on my reading of the module resolution logic, I believe everything is correct, though I'm not totally sure whatrealpathresolves to in the context of this particular Bazel execution, but based on the debug paths spit out by Node, thepackage.jsonappears to be in the right spot and should be used for all nested.jsfiles.I'm not sure what's going on here atm. I think the next step would be to try this same directory structure outside of Bazel to make sure it works the way I expect it to.
Prototype: https://github.com/dgp1130/rules_prerender/commits/ref/esm
At this point though, I'm not convinced this is worth the effort. If it's this hard to get my code compatible, every application using
rules_prerenderwould run into the same issues. There's also no clear benefit to doing this right now. I think for now I'm just going to accept defeat and hope the toolchain solves some of these problems for me later on.The text was updated successfully, but these errors were encountered: