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

tape runner does not support ESM files #514

Closed
donmccurdy opened this issue May 16, 2020 · 33 comments
Closed

tape runner does not support ESM files #514

donmccurdy opened this issue May 16, 2020 · 33 comments

Comments

@donmccurdy
Copy link

Reading #414 — which I realize is about writing Tape with ES modules, not testing ES modules — I found this suggestion:

#414 (comment)

In case someone stumbles upon this, passing --experimental-modules to tape works fine on windows (nix not available or I'd test). Even in a context such as this: npx tape --experimental-modules test/**/.mjs.

Testing now, in node.js v12.9.1 and macOS, I'm getting a silent failure when trying to use that. For example:

import test from 'tape';
// import * as test from 'tape';
// import { test } from 'tape';

test('test', function (t) {
  t.equal( 'test', 'test', 'it is a test' );
  t.end();
});
tape --experimental-modules my-test.js

Should this be supported? It may be worth noting that I also get no output running tape -v or tape -h. I'm not sure if that indicates a problem with my environment.

@ljharb
Copy link
Collaborator

ljharb commented May 16, 2020

You no longer need the experimental modules flag on node v13.2+, fwiw.

Separately, on node 12, prior to v12.16, the experimental modules implementation is incomplete and/or broken. So, if you want to use ESM, you must use node ^12.16 || >= 13.2, with or without the flag. It's not possible to have node v12.9 work properly with ESM.

@donmccurdy
Copy link
Author

Thanks — updating node.js would be ok for me, but I seem to be getting the same result on v13.12.0. Are there any examples of doing this correctly?

Continuing to test the example above:

  • tape my-test.js --> Cannot use import statement outside a module
  • tape --experimental-modules my-test.js --> silent failure
  • tape my-test.mjs --> Must use import to load ES Module

@ljharb
Copy link
Collaborator

ljharb commented May 16, 2020

Ah, here we go :-) ESM must be written in an .mjs file (unless the project's package.json has "type": "module", which I'd discourage. However, tape currently uses require, which can only load CJS files.

As such, you'd currently have to pass a CJS file to the tape runner that used import() to get at the ESM files, and then awaited that promise, which is admittedly super awkward.

You can, of course, run all tape tests with node - ie, node my-test.mjs should work just fine, but only for a single file.

I'm not sure how it'd be possible for the tape runner to both support pre-ESM nodes, and also conditionally use import() to load test files, which is a syntax error in older nodes, but I'm very open to PRs if we can figure out a way to do it.

@ljharb ljharb changed the title Tape fails silently with --experimental-modules tape runner does not support ESM files May 16, 2020
@donmccurdy
Copy link
Author

The ES module imported by my actual project was in a dependency, so changing the file extensions or import syntax would have been tricky for sure. Instead, I was able to get this working well with the esm package:

require = require('esm')(module);

const test = require('tape');
const foo = require('foo'); // 'foo' package contains references to ES modules

test('test', function(t) {
  // ...
});

With that, the usual syntax works fine:

tape my-test.js

It seems to have some limitations with .mjs files and type: "module", but I'd rather avoid both of those things, so this is fine.

Thanks for looking into this! Feel free to close the issue; I don't think any fixes are necessary here except perhaps a tip in the docs?

@ljharb
Copy link
Collaborator

ljharb commented May 16, 2020

Nah, the esm package shouldn't be needed in any ESM-supporting node, and without .mjs or "type": "module", it's simply not ESM, so I'll keep this open to track some kind of effort for the tape runner. Thanks for reporting!

@Raynos
Copy link
Collaborator

Raynos commented May 16, 2020

Should we add a "known to be working example" to the README for ESM ?

Something like node test.mjs will always work with the correct import statement.

We could also open a PR for ./bin/tape.js that if a file ends with .mjs use dynamic import() instead of require and then add tape test/*.mjs to the README

@ljharb
Copy link
Collaborator

ljharb commented May 16, 2020

@Raynos there's no way to conditionally use import() and still support pre-ESM node without eval.

@Raynos
Copy link
Collaborator

Raynos commented May 16, 2020

Wait wtf. I thought import function was just a global function you can check for with type of === undefined

@ljharb
Copy link
Collaborator

ljharb commented May 16, 2020

Absolutely not, it's syntax, not a function.

@Raynos
Copy link
Collaborator

Raynos commented May 16, 2020

Is there a bin feature in npm where you can specify an ESM module bin and a CJS bin ?

Maybe this should be a dual package issue on the npm CLI

@ljharb
Copy link
Collaborator

ljharb commented May 16, 2020

No, there's not - but that's a good feature request for the node ESM modules implementation (although it wouldn't help us to support ESM in the versions already released).

We could do some tricks with a different published package to make it work, perhaps.

@donmccurdy

This comment has been minimized.

@ljharb

This comment has been minimized.

@donmccurdy

This comment has been minimized.

@christiaanwesterbeek
Copy link

Absolutely not, it's syntax, not a function.

I don't mean to confuse and also please ignore this :), but:

There is also a function-like dynamic import(), which does not require scripts of type="module".

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import

@ljharb
Copy link
Collaborator

ljharb commented Jul 1, 2020

@christiaanwesterbeek yes, that's syntax, not a function. You can use it anywhere, but it can only consume modules.

@cowwoc
Copy link

cowwoc commented Oct 22, 2020

I'm not sure how it'd be possible for the tape runner to both support pre-ESM nodes, and also conditionally use import() to load test files, which is a syntax error in older nodes, but I'm very open to PRs if we can figure out a way to do it.

Worse-case scenario, can't you publish two different runner implementations and let users trigger the right one depending on their environment?

@ljharb
Copy link
Collaborator

ljharb commented Oct 22, 2020

@cowwoc that would work if every user either had 100% CJS tests, or 100% ESM tests, but I doubt that will always be the case, since most packages will (and should) have both CJS and ESM files to test.

@cowwoc
Copy link

cowwoc commented Oct 22, 2020

@ljharb In that case, users would have to segment test runs in their build script. They'd run the tape-cjs runner for CJS files followed by tape-esm for ESM files. It's slightly annoying to end up with two separate outputs but it's a start (certainly better than not being able to run at all.)

@ljharb
Copy link
Collaborator

ljharb commented Oct 22, 2020

@cowwoc I suppose i'd be willing to accept a PR that added an option to tape that switched it to ESM mode, and then that lazily required a different file that used import() to access files instead of require - but I'm skeptical that'd actually be much of an improvement, since node test/esm.mjs or similar works just fine already (ie, using node, not tape)

@cowwoc
Copy link

cowwoc commented Oct 22, 2020

@ljharb I am interacting with tape through gulp-tape. It sounds to me like you are saying that the plugin would need to change to invoke the test file directly (node [path]) instead of going through the test runner (tape [path]). Is that correct?

@ljharb
Copy link
Collaborator

ljharb commented Oct 22, 2020

Yes - also, tape takes a glob, but node takes a single file.

@cowwoc
Copy link

cowwoc commented Oct 22, 2020

@ljharb Okay, that's a bit of an issue. I guess we can concatenate the output from multiple runs somehow? And I guess we should expect a performance loss from multiple node runs vs a single run?

Another thing to consider: we're going to have to patch N different tape plugins versus updating the code once in tape directly. The latter sounds like less work in the grand scheme of things.

@ljharb
Copy link
Collaborator

ljharb commented Oct 22, 2020

I mean, you'd have to update all the tape dependencies anyways to support a different binary, or a different command line argument - the only way it'd avoid that work is if tape could seamlessly detect when a file was an ESM module, and handle that case differently.

@cowwoc
Copy link

cowwoc commented Oct 23, 2020

@ljharb We might be able to do that. If you require() a module you get something along the lines of Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /home/travis/build/chrisveness/geodesy/test/dms-tests.js require() of ES modules is not supported.

Maybe you can surround require() with a try-catch block to detect this condition?

@ljharb
Copy link
Collaborator

ljharb commented Oct 23, 2020

That's certainly one alternative; something I'd prefer (but while more reliable, may not be as performant) to make a package that, given a path, determines the parse goal (CJS or ESM) and selects the appropriate loader.

@cowwoc
Copy link

cowwoc commented Oct 23, 2020

That is also a nice approach. Let users provide a regex indicating whether a match is CJS or ESM and go from there. I can see some people separating such code by directory name, others by filename extension.

@ljharb
Copy link
Collaborator

ljharb commented Oct 23, 2020

Nah, it wouldn't and shouldn't be user configurable - it'd be:

  1. does this node version support ESM? if not, CJS.
  2. is this file's extension ".cjs" or ".json"? CJS.
  3. is this file's extension ".mjs"? ESM.
  4. is this file's extension ".js"? if within a package.json with type module, ESM, else, CJS.
  5. CJS.

In other words, node, not users, determine what kind of file it is (based on how the user has named it and the user's package.json).

@cowwoc
Copy link

cowwoc commented Oct 23, 2020

Fine with me.

@aral
Copy link

aral commented Mar 3, 2021

Just a heads up that I published a very basic (30 lines of code) module that runs tests in isolation, one after the other, and works with ESM too. Nothing fancy but it works for me. In case it helps: https://github.com/small-tech/esm-tape-runner

@ljharb
Copy link
Collaborator

ljharb commented Mar 3, 2021

That’s roughly the same technique #547 will use; however, tape supports many node and browser versions beyond the ones that have async/await, or even Promises.

@aral
Copy link

aral commented Mar 3, 2021

Thanks for the validation :) And yeah, that’s why I thought I’d make a quick and separate module. Basically, just scratching my own itch but happy if it ends up scratching anyone else’s too. 🐾

@ljharb
Copy link
Collaborator

ljharb commented Jul 26, 2021

This is fixed by #547.

@ljharb ljharb closed this as completed Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants