Use import() on esm files in supported node versions

[lkmill]

This is a proof-of-concept of using import() to load ESM files in supported node versions (>= 14, >= 13.2.0, >= 12.17.0). The syntax breaking import() call is located in a separate file from bin/tape.js that is conditionally loaded, preventing older Node versions from crashing.

This is currently an unfinished draft. I'm mainly curious to see if you are interested in something like this.

Currently this branch does not use promises to sequentially load the imported files. I have a separate branch, https://github.com/lohfu/tape/blob/esm-support-promises/bin/load-module-esm-support.js#L27-L29, where I have implemented sequential loading but this causes a test repo with a couple of simple test files to fail.

Questions / Caveats

1. What is needed for the sequential loading to work? It seems like tape finishes before all tests are run properly. In a very simple repo with 2 test files all tests in the second file error with not ok 2 test exited without ending: file 2 test 1. The tests simply contain a single t.pass and a t.end. All tests in the tape repo pass.
2. Currently only ${cwd}/package.json is loaded as I was a little reluctant to add a new dependency to find closest package.json... Is this ok? Do you have any preferences?
3. Is there a better way to determine ESM support than checking the node version?
4. nyc instrumentation fails: failed to instrument /home/lohfu/dev/tape/bin/load-module-esm-support.js with error: SyntaxError: 'import' and 'export' may only appear at the top level (14:8)... Can I update tap to get a newer nyc?
5. Currently only .mjs and .js files are loaded using import(). This prevents the use of loaders on other file extensions. In the future an --extension argument will probably be needed in the cli to allow for loaders on custom extensions.
6. Is the bin file ever bundled into browsers? I presume bundlers will add the file with the import to the bundle, which will cause older browsers to SyntaxError.

[codecov-io]

Codecov Report

Merging #547 (bfaec0a) into master (50751db) will decrease coverage by 1.90%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #547      +/-   ##
==========================================
- Coverage   74.65%   72.75%   -1.91%     
==========================================
  Files          19       20       +1     
  Lines         789      800      +11     
  Branches      151      153       +2     
==========================================
- Hits          589      582       -7     
- Misses        200      218      +18     

Impacted Files	Coverage Δ
bin/load-module-esm-support.js	0.00% <0.00%> (ø)
lib/test.js	94.30% <0.00%> (-1.43%)	:arrow_down:
index.js	96.73% <0.00%> (-1.09%)	:arrow_down:
lib/results.js	99.31% <0.00%> (-0.69%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 50751db...bfaec0a. Read the comment docs.

[ljharb]

We also can’t upgrade tap due to the node versions it drops.

[lkmill]

i used a promise reducer to load test files serially and added failing tests. the new tests fail due to the problem i explained in the PR description, namely all tests in the files loaded after the first fail with not ok 2 test exited without ending: module-b

We also can’t upgrade tap due to the node versions it drops.

hmmm, ok. any other way of circumventing the instrumentation error?

[ljharb]

I'm not sure what the instrumentation error is; is it about import()?

[lkmill]

regarding the failing tests, if you point me in the general direction of possible causes i am keen to start debugging and looking for solutions.

I'm not sure what the instrumentation error is; is it about import()?

yes, the exact error is:

failed to instrument /home/lohfu/dev/tape/bin/load-module-esm-support.js with error: SyntaxError: 'import' and 'export' may only appear at the top level (16:8)

[ljharb]

add bin/load-module-esm-support.js to exclude in .nycrc?

[ljharb]

also we'll need to make sure the tests that do imports are skipped in the appropriate node versions.

[lkmill]

@ljharb All tests pass now. If you are ok with the way i wait for all tests to load before running them I reckon the only things left now are:

1. Skipping import tests in appropriate Node versions
2. Deciding whether to check to use require or import for each file or always using import.

[lkmill]

Ok I did some very basic performance testing to see the implications of always using import. I created 10,000 empty .js files with a module type package.json. There was no observable performance benefit of removing the package type check and always using import. However, require:ing the same 10,000 files was around 4 times faster than importing them. Conclusion: no benefits when importing ESM files but serious disadvantage when importing CommonJS files.

[lkmill]

@ljharb what do you think about using this solution instead of checking which version of node is used to figure out if dynamic import is supported?

https://github.com/davidtheclark/cosmiconfig/pull/251/files#diff-e61e569af4ed5c2bd19c648e11225dd502296f0931b339c88cb82288a493e6ad

[ljharb]

@lohfu that's basically the solution i'll be using in the package i forgot to create :-) let me get on that today.

[codecov-commenter]

Codecov Report

Merging #547 (e6d2faf) into master (9bbf270) will increase coverage by 0.06%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #547      +/-   ##
==========================================
+ Coverage   96.70%   96.77%   +0.06%     
==========================================
  Files           4        4              
  Lines         607      620      +13     
  Branches      144      147       +3     
==========================================
+ Hits          587      600      +13     
  Misses         20       20              

Impacted Files	Coverage Δ
index.js	98.11% <100.00%> (+0.26%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9bbf270...e6d2faf. Read the comment docs.

[ljharb]

@lohfu alrighty, give https://npmjs.com/has-dynamic-import a spin

[lkmill]

@lohfu alrighty, give https://npmjs.com/has-dynamic-import a spin

Sweet! Unfortunately I ran into some trouble with it immediately... I use has-dynamic-import to skip the ESM tests, and currently they arent skipped and fail in Node 10, 11, < 12.17 and < 13.2. All else seem to work fine. I created an issue and a PR explaining the issue and possible workaround.

[lkmill]

i have fiddled around a little with getting this PR working with inspect-js/has-dynamic-import#3. all tests seem to be working on my end, but i have the following questions:

1. is the current way of handling async module loading ok? see:
   • https://github.com/substack/tape/pull/547/files#diff-cac4e8fa9811bc382c4e22b2d21dd2bf2b00675c458d17cbc221866db62f078eR55
   • https://github.com/substack/tape/pull/547/files#diff-75d9d2e71dcc077af46469ce4fce3f93e91f27b8f09b82dfa5574c304ee69aa3R86
2. should we have an option for custom extensions that enforces import() like .mjs does now? this would enable using loaders and other spooky magic (https://github.com/substack/tape/pull/547/files#diff-79dfc9fc6fa8fb5312506d39301093b519b95edbb72465a85102b1be7ec0f0f7R9)
3. should i squash/rebase away all "useless" commits or do you want the full history?

[lkmill]

4. since full support check is now async and the tap skip option is sync, i used async sub-tests to exclude import() tests in unsupported environments (https://github.com/substack/tape/pull/547/files#diff-5ab063121df04838565bf235950a645d72c689af2409b48129368b6317bf23b2R8-R52), is this a viable solution?

[ljharb]

When node supports something more, we can add support for it. At the moment, there are no other kinds of modules.

[lkmill]

It occurred to me today that node only has --require and does not have any way to evaluate ESM files on process load - it might be better to wait until node has that support before adding it here, in case they conflict.

removed!

[ljharb]

I'm not sure what you removed - what i mean is, --require=./foo.mjs is impossible in node, so i'm wondering if it should remain impossible in tape.

[lkmill]

i removed the logic to use import() statements on ESM file in the --require option.

i am feeling pretty ready with this PR now... if you are ok with the wait()/run() logic for handling async importing of tests before running them i will update the README and squash everything to a single commit.

[lkmill]

so i started using the changes in this branch on a project, and i've found a major drawback to the handling of rejections when loading test files.

https://github.com/substack/tape/blob/229bd7876ec71d5c8d7681a4f2e6c13cb61a0efe/bin/tape#L67-L73

catching errors in our loading promise, console.error:ing them and then exiting makes it very difficult to locate syntax errors in ESM files. running an ESM file with invalid syntax "natively" in Node prints:

$ node tmp.mjs
file:///home/notme/dev/tape/tmp.mjs:1
)
^

SyntaxError: Unexpected token ')'
    at Loader.moduleStrategy (node:internal/modules/esm/translators:147:18)
    at async link (node:internal/modules/esm/module_job:64:21)

the actual stack trace contains no information of where the syntax error is; it's the lines before that discloses the error and its location. running the same file with this branch prints:

$ ./bin/tape tmp.mjs
SyntaxError: Unexpected token ')'
    at Loader.moduleStrategy (node:internal/modules/esm/translators:147:18)
    at async link (node:internal/modules/esm/module_job:64:21)

with only that information, it obviously becomes difficult to locate the problem without external tools. node appears to behave differently when the syntax errors are in commonjs files, ie when tape calls require() and not import(). in that case, console.error(err) prints the syntax error and it's location:

$ ./bin/tape tmp.cjs
/home/notme/dev/tape/tmp.cjs:1
)
^

SyntaxError: Unexpected token ')'
    at Object.compileFunction (node:vm:353:18)
    at wrapSafe (node:internal/modules/cjs/loader:1039:15)
    at Module._compile (node:internal/modules/cjs/loader:1073:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1138:10)
    at Module.load (node:internal/modules/cjs/loader:989:32)
    at Function.Module._load (node:internal/modules/cjs/loader:829:14)
    at Module.require (node:internal/modules/cjs/loader:1013:19)
    at require (node:internal/modules/cjs/helpers:93:18)
    at module.exports (/home/notme/dev/tape/bin/import-or-require.js:12:5)
    at /home/notme/dev/tape/bin/tape:85:24

[ljharb]

Yeah, i think node does some magic with exceptions that are not caught and re-thrown. I’m not sure how to solve for that.

[lkmill]

After giving it some thought I have come up with some changes. The basic idea is to use process.nextTick to leave the promise chain after checking for import support and only enter into a new promise chain when loading ESM files. This way we avoid unhandled rejections when only loading CommonJS files, ie in all current setups using tape. If we do enter into a promise chain when loading files, rejections are no longer caught. This makes Node 15 onward handle rejections importing files similar to errors requiring files. I know one should usually strive for handling all rejections, but I reckon this is a bit of an edge case. We want the program to quit when there are rejections importing files, and we want to show exactly what Node natively prints. If rejections need to be handled when importing files, shouldn't the current require calls be wrapped in a try/catch?

After these last changes, this PR only affects people trying to load ESM files in versions that supports it and a loading promise will only be created if an ESM file is imported. This completely avoids unhandled rejections when using CommonJS and will only have awkward output in Node versions before 15. People converting their projects to full ESM most likely use very modern Node and as such, this should have poor behaviour for few. Moreover, 99% of unhandled rejections importing files will be syntax errors and developers have tools in place to locate those.

On rejections importing ESM files, the previous changes printed::

SyntaxError: Unexpected token ')'
    at Loader.moduleStrategy (internal/modules/esm/translators.js:84:18)
    at async link (internal/modules/esm/module_job.js:36:21)

Evidently this output gives very little clue as to were the error has occurred. As just mentioned, usually these errors will easily be solved with other developer tools, but in rarer cases not getting an exact location where the error was thrown can be struggling.

After the last changes, promise rejections importing ESM files prints:

Node >=15:

file:///home/notme/dev/tape/tmp.mjs:1
)
^

SyntaxError: Unexpected token ')'
    at Loader.moduleStrategy (node:internal/modules/esm/translators:147:18)
    at async link (node:internal/modules/esm/module_job:48:21)

Node ^12.17, ^13.2 and 14:

(node:91315) UnhandledPromiseRejectionWarning: SyntaxError: Unexpected token ')'
    at Loader.moduleStrategy (internal/modules/esm/translators.js:84:18)
    at async link (internal/modules/esm/module_job.js:36:21)
(node:91315) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:91315) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

All Node versions that do not support import:

/home/notme/dev/tape/tmp.mjs:1
)
^

SyntaxError: Unexpected token )
    at Module._compile (internal/modules/cjs/loader.js:760:23)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:827:10)
    at Module.load (internal/modules/cjs/loader.js:685:32)
    at Function.Module._load (internal/modules/cjs/loader.js:620:12)
    at Module.require (internal/modules/cjs/loader.js:723:19)
    at require (internal/modules/cjs/helpers.js:14:16)
    at /home/lohfu/dev/tape/bin/tape:86:34
    at Array.forEach (<anonymous>)
    at importFiles (/home/lohfu/dev/tape/bin/tape:86:11)
    at processTicksAndRejections (internal/process/task_queues.js:81:17)

For errors requiring CJS files, the output has always been the same both inside and outside promises (both caught and uncaught).

[ljharb]

If rejections need to be handled when importing files, shouldn't the current require calls be wrapped in a try/catch?

It's not that they need to be handled; it's that module-level exceptions (at require/import time) should crash the process; unhandled rejections (including from an ESM module that does export function then() { throw 42 }) should not.

Ideally, a module-level exception from a CJS file should look the same in any version of node, whether or not you're including ESM files.

Your last comment implies we might be pretty close to the ideal scenario :-) I'll take a look!

[lkmill]

Since playing around a little with the --unhandled-rejections=mode CLI option I am getting quite convinced this is a reasonable solution. Using the NODE_OPTIONS env variable to set unhandled rejections mode to either strict or throw, Node and tape handles unhandled rejections the same in all version of Node that supports ESM imports.

$ NODE_OPTIONS='--unhandled-rejections=throw' ./bin/tape tmp.mjs
file:///home/notme/dev/tape/tmp.mjs:1
)
^

SyntaxError: Unexpected token ')'
    at Loader.moduleStrategy (node:internal/modules/esm/translators:147:18)
    at async link (node:internal/modules/esm/module_job:48:21)

I added some tests to check how the process exits, but do not really have any tests checking the error output.

EDIT:

Only Node >=14 seems to support throw mode, earlier version only support strict. in the tests i had to set it to strict mode to make node 12 pass and 13.

EDIT:

Added some tests checking for unhandled rejections in the stderr.

[lkmill]

@ljharb have you had any time to review the latest changes?

[ljharb]

@lohfu thanks for your patience and for working so thoroughly on this, I really appreciate it!

I'll cut a release that includes this soon.

[ljharb]

See v5.3.0

[ghost]

Just here to say that this is amazing, thank you everyone 🥳