Support loading ESM test files #2382

arlac77 · 2020-01-26T21:18:11Z

Brute force try import(ref)
at least works for me

How to make xo not complain about fully qualified imports ?

import test from '../../index.js';

sindresorhus · 2020-01-27T01:04:47Z

You need to await the import call. Otherwise, the try/catch has no effect.

arlac77 · 2020-01-27T01:34:24Z

You need to await the import call. Otherwise, the try/catch has no effect.

the try catch is there to prevent the unknown import() function error (node < 10?)
and fall through to the former error handling

novemberborn

Nice! Big picture the current error message needs to be updated for when ESM is not available, cause with this PR AVA does support loading ESM test files.

It'd be good to have a test for when ESM does work, too.

test/fixture/esm/test.js

lib/worker/subprocess.js

test/integration/assorted.js

novemberborn · 2020-01-27T08:03:36Z

How to make xo not complain about fully qualified imports ?
import test from '../../index.js';

The XO output should contain the failing rule. Then after that line add // eslint-disable-line the-failing-rule and the rule should be ignored at that point.

novemberborn

Thanks for the updates @arlac77!

lib/worker/subprocess.js

test/integration/assorted.js

…ion" This reverts commit d8fdba3.

novemberborn · 2020-02-02T13:28:56Z

@arlac77 great find with the path vs URL stuff!

I think there's two remaining issues:

When import() is not available, the error message shown should communicate that. Currently it says AVA doesn't support ESM.
Errors thrown in the imported module should be reported, like they are for CJS modules.

arlac77 · 2020-02-02T16:23:46Z

like they are for CJS modules.

Where can I find CJS module error code ?

novemberborn · 2020-02-02T16:30:24Z

like they are for CJS modules.

Where can I find CJS module error code ?

No, the issue is the catch {} statement. You're assuming the only thrown error is because Node.js does not support ESM. But you're inadvertently swallowing errors thrown by the module.

Instead I think the safest way to deal with this is to explicitly detect ESM support by importing a known .mjs file included with AVA itself. Then in the load() function you can check for support before calling import(). If it's missing, emit the internal error and exit. Otherwise import without swallowing errors.

novemberborn

We should also update https://github.com/avajs/ava/blob/8fa28254dbebef32cbde05c0c9a49061d0ef82f8/docs/recipes/es-modules.md which currently states that ESM files cannot be loaded.

lib/worker/subprocess.js

novemberborn · 2020-02-04T08:52:20Z

Thanks @arlac77. This looks good at first glance, but it'll probably take until the weekend before I can take a closer look.

Rename to supportsESM.

novemberborn · 2020-02-09T15:00:20Z

@arlac77 this is great! I've made some tweaks and updated the documentation. Will merge when CI passes 🎊

fisker · 2020-02-18T03:26:11Z

lib/worker/subprocess.js

 	let requireFn = require;
 	const load = async ref => {
 		for (const extension of extensionsToLoadAsModules) {
 			if (ref.endsWith(`.${extension}`)) {
-				ipc.send({type: 'internal-error', err: serializeError('Internal runner error', false, new Error('AVA cannot yet load ESM files.'))});
+				if (await supportsESM()) { // eslint-disable-line no-await-in-loop


I think

if (cachedSupportsESMResult || await supportsESM())

can make this loop faster.

arlac77 added 5 commits January 26, 2020 22:05

fix: use dynamic import() to load esm modules

9e79ccf

style: fix some xo styling issues

c8c9b22

style: xo is happy now

6873e32

test: differentiate module loading asserts based on node version (13)

c886006

test: use fully qualified imports (index.js) in esm fixtures

bba6daf

novemberborn requested changes Jan 27, 2020

View reviewed changes

test/fixture/esm/test.js Outdated Show resolved Hide resolved

lib/worker/subprocess.js Outdated Show resolved Hide resolved

test/integration/assorted.js Outdated Show resolved Hide resolved

novemberborn changed the title ~~fix: use dynamic import() to load esm modules~~ Support loading ESM test files Jan 27, 2020

arlac77 added 6 commits January 27, 2020 10:02

style: add hints to make import '.../index.js' pass

bc7a7d6

style: use optional catch binding

4103261

fix: force esm loading exceptions to be catched by load function

d8fdba3

debug: node@13/windows import error

4a5c3fb

test: asume esm loading on node 13@win32 does not work for now

f327ca0

test: new wording for esm load tests

db441ee

novemberborn reviewed Feb 1, 2020

View reviewed changes

lib/worker/subprocess.js Outdated Show resolved Hide resolved

test/integration/assorted.js Outdated Show resolved Hide resolved

arlac77 and others added 3 commits February 1, 2020 19:52

Revert "fix: force esm loading exceptions to be catched by load funct…

b0fa2ba

…ion" This reverts commit d8fdba3.

fix: convert path to url when using import()

6946d74

style: make xo happy

531eb1e

fix: wording on when node does not support import()

f11ee1e

fix: check for import() with well known module

d496118

arlac77 requested a review from novemberborn February 2, 2020 20:06

Merge branch 'master' of https://github.com/avajs/ava

72fdd72

novemberborn reviewed Feb 3, 2020

View reviewed changes

lib/worker/subprocess.js Outdated Show resolved Hide resolved

lib/worker/subprocess.js Outdated Show resolved Hide resolved

lib/worker/subprocess.js Show resolved Hide resolved

lib/worker/subprocess.js Outdated Show resolved Hide resolved

Markus Felten and others added 2 commits February 3, 2020 12:53

perf: move import() detection logic out of loop

2579067

fix: error message wording

08c14b6

arlac77 requested a review from novemberborn February 3, 2020 13:33

novemberborn added 7 commits February 9, 2020 15:28

Use .mjs extension for probe, update comments

e7d33dd

No need to await the import (my bad)

530a7fc

Tweak error message

dc743f6

Rename userland esm package fixture

3ad38d4

Rename type=module ESM fixture

71457dd

Lazily check for dynamic import support

3cbc4e6

Rename to supportsESM.

Update ES module recipe

52e11cc

novemberborn approved these changes Feb 9, 2020

View reviewed changes

novemberborn merged commit 04ba44b into avajs:master Feb 9, 2020

fisker reviewed Feb 18, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support loading ESM test files #2382

Support loading ESM test files #2382

arlac77 commented Jan 26, 2020 •

edited by novemberborn

Loading

sindresorhus commented Jan 27, 2020

arlac77 commented Jan 27, 2020 •

edited

Loading

novemberborn left a comment

novemberborn commented Jan 27, 2020

novemberborn left a comment

novemberborn commented Feb 2, 2020

arlac77 commented Feb 2, 2020

novemberborn commented Feb 2, 2020

novemberborn left a comment

novemberborn commented Feb 4, 2020

novemberborn commented Feb 9, 2020

fisker Feb 18, 2020

Support loading ESM test files #2382

Support loading ESM test files #2382

Conversation

arlac77 commented Jan 26, 2020 • edited by novemberborn Loading

sindresorhus commented Jan 27, 2020

arlac77 commented Jan 27, 2020 • edited Loading

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn commented Jan 27, 2020

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn commented Feb 2, 2020

arlac77 commented Feb 2, 2020

novemberborn commented Feb 2, 2020

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn commented Feb 4, 2020

novemberborn commented Feb 9, 2020

fisker Feb 18, 2020

Choose a reason for hiding this comment

arlac77 commented Jan 26, 2020 •

edited by novemberborn

Loading

arlac77 commented Jan 27, 2020 •

edited

Loading