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

The ESM Plan #2293

Closed
12 tasks done
novemberborn opened this issue Nov 10, 2019 · 23 comments
Closed
12 tasks done

The ESM Plan #2293

novemberborn opened this issue Nov 10, 2019 · 23 comments

Comments

@novemberborn
Copy link
Member

novemberborn commented Nov 10, 2019

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 cjs and mjs extensions. Currently, by default, we only look for files with js extensions.

  • When loading test files, we'll always load files with cjs extensions using require(). Files with mjs extensions will be loaded using import(). Support loading .mjs test files #2344

  • Note 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 if import() is not available and AVA is trying to load an mjs test file.

  • When it comes to js files we'll follow the package.json "type" field. So by default we'll treat them as CJS.

  • We'll also let you configure how js files (and other files, but not cjs or mjs files) 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 #2345

  • The 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.cjs and ava.config.mjs configuration files. However deciding what to do with ava.config.js files is more difficult. We shouldn't change how it's loaded based on "type" fields, nor can we use the built-in import() 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 #2346

  • I'm proposing we remove support for import statements and require() calls from ava.config.js files. In other words, they must only contain export default {} or export default () => ({}) style configurations. This is a breaking change, as at the moment we load ava.config.js using the esm package. 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 require modules, 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 and import() is available then that should work. Else, for now, we'll fall back to using require(). Update our handling of the require option to support ESM dependencies #2347

  • Dependency tracking for our watch mode also needs work. Make dependency tracking work with ESM imports #2388

@sindresorhus
Copy link
Member

We shouldn't change how it's loaded based on "type" fields

Why not?

@novemberborn
Copy link
Member Author

We shouldn't change how it's loaded based on "type" fields

Why not?

First of, Node.js defaults to CJS for js files. Following this default breaks all existing configuration files. With my suggestion to only support export default most simple files will still work.

Changing the "type" field would have the consequence of breaking your AVA configuration, which I'd find surprising.

Finally, if we say you can override how js files are loaded, through configuration, it's confusing that you can't change how the ava.config.js file itself is loaded.

@novemberborn
Copy link
Member Author

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.

@clitetailor
Copy link

IMO, why don't we support Node experimental modules under a flag. For example:

ava --experimental-modules

So 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.

@novemberborn
Copy link
Member Author

IMO, why don't we support Node experimental modules under a flag. For example:

ava --experimental-modules

So 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.

@fregante

This comment has been minimized.

@novemberborn
Copy link
Member Author

@fregante,

It appears that AVA can't be used on Node 13 with type: module in package.json.

Yup. This issue sets out our approach to making it work.

shreyasminocha added a commit to shreyasminocha/decode-binary-bot that referenced this issue Dec 19, 2019
They're still failing, however, due to an ava deficiency.
See avajs/ava#2293.
@novemberborn novemberborn added this to the v3 milestone Dec 21, 2019
novemberborn added a commit that referenced this issue Jan 4, 2020
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
@novemberborn
Copy link
Member Author

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.

@novemberborn novemberborn removed the breaking requires a SemVer major release label Jan 5, 2020
@novemberborn novemberborn removed this from the v3 milestone Jan 5, 2020
@lorenzofox3
Copy link

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 esm with my test runner for some time now and I am very happy.

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.

ava -r esm some/test.js

@novemberborn
Copy link
Member Author

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.

@fregante
Copy link

esm is no longer enough in Node 13, at least until this is fixed standard-things/esm#855

@novemberborn
Copy link
Member Author

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.

@haywirez
Copy link

Any current recipes? My entire codebase uses import statements etc., but I cannot seem to quickly figure out how it's suppose to work (running Node 14.6). My package.json has "type": "module", but module resolution doesn't seem to work inside tests.

@novemberborn
Copy link
Member Author

@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?

warner added a commit to Agoric/agoric-sdk that referenced this issue Aug 27, 2020
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.
@iambumblehead
Copy link

@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

  "scripts" : {
    "unit-test": "ava --node-arguments=\"--loader=esmock\""
  }

If a --loader option were supported something like this might work

  "scripts" : {
    "unit-test": "ava --loader=esmock"
  }

@novemberborn
Copy link
Member Author

@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.

@tcollinsworth
Copy link

tcollinsworth commented Jan 22, 2022

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.
The esm library does not yet support optional chaining (elvis operator). The esm-wallaby library which is a fork of esm does.
It's disappointing that module support hasn't addressed these issues and we can't move to type:module, .mjs or ava 4.x until these are resolved. For that reason I'm not updating libraries I maintain to type: module or .mjs until these issues are resolved. Actually I did update them to type:module and .mjs, but backed all the changes out after discovering the issues and no workarounds. Consumer of my libs can use type:module, .mjs, or use esm, esm-wallaby, or some other shim (lots of options). They aren't blocked from updating like ava 4.x currently.

@novemberborn
Copy link
Member Author

@tcollinsworth AVA 4 works just fine with CJS. Being a Node.js test runner, for ESM support we rely on Node.js' standard behavior.

@iambumblehead
Copy link

@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.

@drmrbrewer
Copy link

@iambumblehead any suggestions for an esm alternative to newrelic?

@iambumblehead
Copy link

@drmrbrewer I'm not a newrelic user and don't know of any suggestions (sorry)

@Primajin
Copy link

@drmrbrewer check out https://github.com/getsentry

@novemberborn
Copy link
Member Author

With #3218 watch mode now does dependency tracking of ES modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants