Support ECMAScript modules (ESM) #251
Conversation
Codecov Report
@@ Coverage Diff @@
## v8 #251 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 9
Lines 227 235 +8
Branches 46 47 +1
=========================================
+ Hits 227 235 +8
Continue to review full report at Codecov.
|
|
Known to-dos:
|
|
By using the |
| @@ -16,6 +17,18 @@ const loadJs: LoaderSync = function loadJs(filepath) { | |||
| return result; | |||
| }; | |||
|
|
|||
| const loadJs: LoaderAsync = async function loadJs(filepath) { | |||
| // This logic is validated by tests running in Node versions that don't | |||
| // support dynamic imports, like Node 10. | |||
There was a problem hiding this comment.
Is it specific versions of Node 10 that don't support dynamic import syntax?
Using this site, 9.2.0 gives a SyntaxError, but 10.1.0 doesn't.
I think this will fail when the syntax is supported, but the feature is not (f.e. when --experimental-modules is needed).
Since v10 EOL is just around the corner, it's probably easier to just wait until then (2021-04-30), and drop support for it.
There was a problem hiding this comment.
Since v10 EOL is just around the corner, it's probably easier to just wait until then (2021-04-30), and drop support for it.
I would just drop support for Node.js 10 now. It's 10 days left. This will not be released until then anyway.
| // support dynamic imports, like Node 10. | ||
| /* istanbul ignore else */ | ||
| if (canUseDynamicImport()) { | ||
| const imported = await import(filepath); |
There was a problem hiding this comment.
An import call will syntax error in node versions that do not support it. In a PR I'm fiddling with to support ESM in tape I side step this syntax error by conditionally requiring another file containing the import statement...
The conditional require:
https://github.com/lohfu/tape/blob/esm-support/bin/tape#L29
The separate file:
https://github.com/lohfu/tape/blob/esm-support/bin/load-module-esm-support.js
Since dynamic imports seem to be up to 5 times slower than require, we conditionally use import or require by checking file extensions and get-package-type. As you probably load far less files this might not be necessary for you.
There was a problem hiding this comment.
If you dont want to conditionally use require vs import, you could just use the function you create to check dynamic import support, https://github.com/davidtheclark/cosmiconfig/pull/251/files#diff-e61e569af4ed5c2bd19c648e11225dd502296f0931b339c88cb82288a493e6adR6.
ie something like:
let dynamicImport;
try {
dynamicImport = new Function('id', 'return import(id);');
} catch (e) {}
// later
const loadJs: LoaderAsync = async function loadJs(filepath) {
if (dynamicImport) {
const imported = await dynamicImport(filepath);
return imported.default;
} else {
return loadJsSync(filepath, null);
}
};There was a problem hiding this comment.
it appears it's even trickier... versions 10.0.0 - 12.16 and 13.0 - 13.1 support the import keyword, but will throw a "Not Supported" error when called. See inspect-js/has-dynamic-import#2 for an attempted work around.
|
The segfault issue is probably caused by nodejs/node#35889 |
|
Just posted this note at the top of the README:
If you're interested in taking ownership of the package and moving this feature forward, let me know! |
|
This was superseded by #304, which is now merged. |
This PR intends to close #224. I don't have more time to work on this today so I'll get what I have up as a draft — feedback welcome! I'd especially appreciate help testing this from people interested in using ESM, both ensuring that it works as expected and ensuring that the expectations are the right ones.
Some notes:
import()function..mjsis only added to the defaultsearchPlacesof the asynchronous API.mastertomain). We'll see.jest-circustest runner. Turns out that wasn't the problem at all, but I left in that update.v8branch, in case there's anything else in the issue queue that anybody wants to get into v8.