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
Require Node.js 12.20 and move to ESM #478
Conversation
Yes, that's what I would go with, but let's wait for @ehmicky's opinion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for working on this @NMinhNguyen, this is super awesome! That's a huge amount of work.
I reviewed it thoroughly and this is looking pretty close to perfect to me, except a few points:
- Renaming the exported function names. The function names you've suggested sound good to me too. I am wondering if it might worth doing in a second/follow-up PR, to minimize the diff?
- Using named imports in all test files.
- Small thing about a specific test file.
Thanks a lot for focusing this PR only on the conversion itself, without changing any logic, except for dependencies upgrade, that's very helpful. ❤️
Thanks for the thorough review @ehmicky! I'll see if I can split the changes into a more reviewable diff when I get home tonight. |
Note: I think only the exported function names would need to be split into a separate PR, and that renaming has not been done yet. In particular, we do not need to split a PR for the |
Ah I meant I'd try to split this single commit into smaller ones for a better diff, unless you've already thoroughly reviewed all the changes? I find that |
Breaking it down to smaller commits might help @sindresorhus, since he might want to do a second round of review before we merge this PR. But as far as I am concerned, I've reviewed all of this PR, so I would not benefit from breaking down the commits. |
@ehmicky I've pushed separate commits (so you can review them more easily) to
I believe that these changes aren't too bad to review now (remember to use |
This works like this, thanks @NMinhNguyen! The PR looks good to me now 👍 |
Pushed a change that restores running tests against Node.js 12. @ehmicky let me know if there's anything further you'd like me to do. |
There are a couple of outdated references:
|
It would also be great if you could remove all the |
What should this be updated to since there's no By the way, I found another way to rewrite the test('should work with third-party Promise', async t => {
// Introduce an artificial delay to avoid breaking `ava`.
await Promise.resolve();
// Can't use `test.before`, because `ava` needs `Promise`.
const nativePromise = Promise;
global.Promise = class BrokenPromise {
then() {
throw new Error('error');
}
};
const {execa} = await import('../index.js');
global.Promise = nativePromise;
const {stdout} = await execa('noop.js', ['foo']);
t.is(stdout, 'foo');
}); |
|
We can just keep the existing workaround until we can target Node.js 14. |
Thanks for working on this, @NMinhNguyen 👍 |
Apologies for a huge PR but it's best to review ignoring whitespace: https://github.com/sindresorhus/execa/pull/478/files?w=1
I tried to follow the following resources:
Opening this as a draft PR since I'm not 100% sure of some of the changes:
What should the named exports be forPrefixed the named exports withcommand
,commandSync
andnode
? Should they beexecaCommand
,execaCommandSync
andexecaNode
?execa
.nyc
toc8
to work around Nyc on ESM Node.js 13 (no babel) istanbuljs/nyc#1287Removed Node.js 12 from the test matrix as I've changedChanged the test inrequire
toimport()
+ top-level await (TLA) inmain...NMinhNguyen:esm?expand=1&w=1
#diff-8f4f0a53c9, and TLA isn't supported in Node.js 12.aefa85c
(#478) to ensure tests can still be run in Node.js 12..js
extension. Alternatively, I could've added apackage.json
with"type": "commonjs"
.TODOs
.js
extension