Require Node.js 12.20 and move to ESM

[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:

• https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c
• https://github.com/sindresorhus/typescript-definition-style-guide
• sindresorhus/is-stream@5831295?w=1.

Opening this as a draft PR since I'm not 100% sure of some of the changes:

• What should the named exports be for command, commandSync and node? Should they be execaCommand, execaCommandSync and execaNode? Prefixed the named exports with execa.
• Had to switch from nyc to c8 to work around Nyc on ESM Node.js 13 (no babel) istanbuljs/nyc#1287
• Removed Node.js 12 from the test matrix as I've changed require to import() + top-level await (TLA) in main...NMinhNguyen:esm?expand=1&w=1#diff-8f4f0a53c9, and TLA isn't supported in Node.js 12. Changed the test in aefa85c (#478) to ensure tests can still be run in Node.js 12.
• Moved the fixtures to ESM which required adding a .js extension. Alternatively, I could've added a package.json with "type": "commonjs".

TODOs

• Ensure all fixtures are referenced using .js extension

[sindresorhus]

What should the named exports be for command, commandSync and node? Should they be execaCommand, execaCommandSync and execaNode?

Yes, that's what I would go with, but let's wait for @ehmicky's opinion.

[ehmicky]

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. ❤️

[NMinhNguyen]

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.

[ehmicky]

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 xo update, per our above discussion.

[NMinhNguyen]

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 xo update, per our above discussion.

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 ?w=1 really helps in these situations.

[ehmicky]

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.

[NMinhNguyen]

There are a couple of outdated references:

test/stdio.js
51:forkMacro.title = macroTitle('execa.fork()');

What should this be updated to since there's no execaFork?

By the way, I found another way to rewrite the override-promise.js test such that it works in Node.js 12 but I'm not sure if it's preferable as it's a little bit hacky:

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');
});

[sindresorhus]

What should this be updated to since there's no execaFork?

execaNode

[sindresorhus]

By the way, I found another way to rewrite the override-promise.js test such that it works in Node.js 12 but I'm not sure if it's preferable as it's a little bit hacky:

We can just keep the existing workaround until we can target Node.js 14.

[sindresorhus]

Thanks for working on this, @NMinhNguyen 👍