The ESM Plan #2293
Comments
novemberborn
added
breaking
Why not? |
First of, Node.js defaults to CJS for Changing the Finally, if we say you can override how |
|
I've also just realized our dependency tracking (for the watcher) won't work for ESM. Perhaps we can use the loader plugins, or else we should try and do static analysis. The latter is complicated by new JavaScript & TypeScript syntaxes. |
|
IMO, why don't we support Node experimental modules under a flag. For example: ava --experimental-modulesSo that it won't break the current behavior and we can switch to the new modules system when the interoperability and the loader hooks are stable enough. |
#2272 will add a nicer way of defining the Node.js flags applied to the worker processes than what is currently possible. However, ESM is available already without a flag in Node.js 13. We just need to make a bunch of changes to make effective use of it. And yes it would be opt-in, but that's largely due to how the feature is designed in Node.js itself. |
This comment has been minimized.
This comment has been minimized.
Yup. This issue sets out our approach to making it work. |
They're still failing, however, due to an ava deficiency. See avajs/ava#2293.
As per #2293, this begins the work to support `.cjs` and `.mjs` files. * AVA will now select and load `.cjs` test files * AVA will now select, but refuse to load, `.mjs` test files * AVA will detect the module `"type"` in `package.json`, and refuse to load `.js` test files if the type is `"module"` * `.js` config files can now only contain an `export default` statement * `.cjs` config files are now supported * `.mjs` config files are now recognized, but AVA refuses to load them * `--config` files must have a known extension
|
I have made all the breaking changes needed for this as part of our v3 release. However we haven't yet added any actual support for ESM files. Please see https://github.com/orgs/avajs/projects/2 for the issues where you can help out. |
|
Have you considered using the esm package ? Native ESM module with node is fine, but when you start mixing with old dependencies or use an older version of node, you will quickly bump into problems. I have been using You don't even need to ship it as a dependency, if ava allows require hooks like node does you can pass it as a command argument.
|
|
Yes that’s been supported for a long time. You can configure the required modules in AVA’s configuration and well then use it to load all subsequent files. It’s only a stop gap solution though. |
|
|
|
All the necessary breaking changes have already gone out in AVA 3.0. The remaining work has been broken up into individual issues that can be worked on. See the ESM support project for details. |
|
Any current recipes? My entire codebase uses |
|
@haywirez I think that should work fine. Perhaps you could open a new issue and include additional details, like your AVA version, and a reproduction? |
We were seeing random test failures in the zoe unit tests (specifically
test-offerSafety.js) that looked like:
```
Uncaught exception in test/unitTests/test-offerSafety.js
/home/runner/work/agoric-sdk/agoric-sdk/packages/weak-store/src/weakStore.js:5
import { assert, details, q } from '@agoric/assert';
^^^^^^
SyntaxError: Cannot use import statement outside a module
✖ test/unitTests/test-offerSafety.js exited with a non-zero exit code: 1
```
or:
```
Uncaught exception in test/unitTests/test-offerSafety.js
/home/runner/work/agoric-sdk/agoric-sdk/packages/zoe/test/unitTests/setupBasicMints.js:1
import { makeIssuerKit } from '@agoric/ertp';
SyntaxError: Cannot use import statement outside a module
✖ test/unitTests/test-offerSafety.js exited with a non-zero exit code: 1
```
under various versions of Node.js. The error suggests that `-r esm` was not
active (the error site tried to import an ESM-style module, and failed
because the import site was CJS not ESM), however error site itself was in a
module, meaning `-r esm` was active up until that moment. This makes no
sense.
The AVA runner has had some amount of built-in support for ESM, in which it
uses Babel to rewrite test files (but perhaps not the code being tested). If
that were in effect, it might explain how `test-offerSafety.js` could be
loaded, but `weakStore.js` could not. It is less likely to explain how
`setupBasicMints.js` could be loaded but `makeIssuerKit` could not, unless
AVA somehow believes that `setupBasicMints.js` is a different category of
file than the ones pulled from other packages.
In any case, I believe we're bypassing that built-in/Babel support, by using
their recommended `-r esm` integration recipe (package.json has
`ava.require=['esm']`). However the AVA "ESM Plan"
(avajs/ava#2293) is worth reading, if only
for our future move-to-native-ESM plans (#527).
So my hunch here is that the `-r esm` module's cache is not safe against
concurrent access, and AVA's parallel test invocation means there are
multiple processes reading and writing to that cache in parallel. Zoe has
more source files than most other packages, which might increase the
opportunity for a cache-corruption bug to show up. This sort of bug might not
show up locally because the files are already in the cache, whereas CI may
not already have them populated.
This patch adds `ESM_DISABLE_CACHE=true` to the environment variables used in
all tests, in an attempt to avoid this hypothetical bug. Stale entries in
this cache has caused us problems before, so most of us have the same setting
in our local shells.
Another potential workaround would be to add `--serial` to the `ava`
invocation, however the Zoe test suite is large enough that we really to want
the parallelism, just to make the tests finish faster.
This patch also increases the Zoe test timeout to 10m, just in case. I
observed a few tests taking 1m30s or 1m40s to complete, and the previous
timeout was 2m, which was too close to the edge.
|
@novemberborn would you consider adding support for a --loader option to the new ava? In order to use this esmock package, for example, one uses something like this inside package.json If a --loader option were supported something like this might work |
|
@iambumblehead as far as I know, loaders are still experimental: https://nodejs.org/api/esm.html#esm_loaders When that changes we can consider how best to expose it in AVA. |
|
Using type: module or .mjs prevents agents like newrelic from instrumenting code or sinon from mocking and jest doesn't support modules fully either, see their ecmascript-modules page. |
|
@tcollinsworth AVA 4 works just fine with CJS. Being a Node.js test runner, for ESM support we rely on Node.js' standard behavior. |
|
@tcollinsworth stop using non-native esm and stop using tools that don't support esm. The only tool you listed that doesn't have a native-esm replacement is newrelic newrelic/node-newrelic#553 If you have control of tools you are using, I would recommending dropping newrelic from your setup because using native esm simplifies so many other things within a package -- smaller download/install size, better stack traces, less configuration and dot files etc. Using native esm makes everything easier. |
|
@iambumblehead any suggestions for an esm alternative to newrelic? |
|
@drmrbrewer I'm not a newrelic user and don't know of any suggestions (sorry) |
|
With #3218 watch mode now does dependency tracking of ES modules. |
Here's what we're doing to support ESM in AVA. The work has already been broken up into individual issues that you can work on. See the ESM support project for details.
See https://nodejs.org/docs/latest/api/esm.html for the latest on what Node.js is implementing (this issue was opened when that documentation was for Node.js 13.1.0). I'm expecting this to become available without a flag at some point.
Firstly, we're moving AVA's Babel support into a separate package that you can install alongside AVA. This will reduce confusion as to what module format AVA supports out of the box. It also allows us to evolve that package independently, which is important since loader hooks may still take some while to land.
Secondly, we'll be detecting test files with
cjsandmjsextensions. Currently, by default, we only look for files withjsextensions.When loading test files, we'll always load files with
cjsextensions usingrequire(). Files withmjsextensions will be loaded usingimport(). Support loading .mjs test files #2344Note that
import()itself is currently behind an experimental flag. Allow Node arguments to be configured #2272 will let you configure that flag. We'll print a useful error message ifimport()is not available and AVA is trying to load anmjstest file.When it comes to
jsfiles we'll follow thepackage.json"type"field. So by default we'll treat them as CJS.We'll also let you configure how
jsfiles (and other files, but notcjsormjsfiles) should be loaded. This way you could override the"type"field, or load test files as ESM even if you're publishing CJS. Add configuration to specify how extensions should be loaded #2345The Babel provider will detect when you've configured files to be loaded as ESM even though it cannot load them as such, and print an error.
We'll support
ava.config.cjsandava.config.mjsconfiguration files. However deciding what to do withava.config.jsfiles is more difficult. We shouldn't change how it's loaded based on"type"fields, nor can we use the built-inimport()since it needs to be enabled. And finally, right now we need to be able to load it synchronously for use with our ESLint plugin. Support ava.config.mjs files #2346I'm proposing we remove support for
importstatements andrequire()calls fromava.config.jsfiles. In other words, they must only containexport default {}orexport default () => ({})style configurations. This is a breaking change, as at the moment we loadava.config.jsusing theesmpackage. However it gives us the best upgrade path.We do need to consider what syntax we use in our documentation. I think we should use CJS, until ESM is available without a flag. It's not as hip, but at least it'll work out of the box.
When loading
requiremodules, if they are loaded from the project itself we can use the above logic to load the files based on their extension. If they're packages andimport()is available then that should work. Else, for now, we'll fall back to usingrequire(). Update our handling of therequireoption to support ESM dependencies #2347Dependency tracking for our watch mode also needs work. Make dependency tracking work with ESM imports #2388
The text was updated successfully, but these errors were encountered: