Skip to content
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

Remove faulty emulated ENOENT error on Windows #447

Merged
merged 1 commit into from
Dec 3, 2020

Conversation

mterrel
Copy link
Contributor

@mterrel mterrel commented Nov 25, 2020

Old behavior

Prior to this PR, execa on Windows would emulate behavior seen on POSIX platforms where attempting to run a non-existent command would throw an error that had code set to ENOENT. However, this emulation worked incorrectly for many common cases (fixes #446 and moxystudio/node-cross-spawn#104), especially when the shell option is set to a truthy value.

New behavior

There is no obvious way to fix the emulation of ENOENT on Windows in a reasonable way, so this PR removes it. That means that execa will no longer throw ENOENT on Windows, but still will on POSIX platforms.
After this PR is merged, when execa is given a non-existent command to run on Windows, execa will have the default Node.js spawn behavior of throwing an error that has exitCode set to 1 and any error message given by Windows will appear in the stderr output/stream.

Detecting "command not found" on Windows

On English versions of Windows, the error message given by the OS looks similar to this:

'somebadcommand' is not recognized as an internal or external command,
operable program or batch file.

However, this message will likely be translated on non-English versions of Windows.

Additional info

Prior to this PR, there were two failing test cases on Windows. Removing the faulty ENOENT emulation actually fixed those two test cases, but caused two new ones to break. I fixed both by inducing a different error.

test/error.js Outdated
const {originalMessage} = await t.throwsAsync(execa('wrong command'));
t.is(originalMessage, 'spawn wrong command ENOENT');
const {originalMessage} = await t.throwsAsync(execa('noop', {cwd: 1}));
t.regex(originalMessage, /The "options.cwd" property must be of type string. Received type number/);
Copy link
Collaborator

@ehmicky ehmicky Nov 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion might not need a regular expression.

Suggested change
t.regex(originalMessage, /The "options.cwd" property must be of type string. Received type number/);
t.true(originalMessage.startsWith('The "options.cwd" property must be of type string'));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as suggested

test/error.js Outdated
if (process.platform !== 'win32') {
test('error.code is defined on failure if applicable', async t => {
const {code} = await t.throwsAsync(execa('invalid'));
t.is(code, 'ENOENT');
Copy link
Collaborator

@ehmicky ehmicky Nov 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be ways to fix this test in a way that works on Windows as well.
We need to find an error.code that is cross-platform.

https://github.com/nodejs/node/blob/master/lib/child_process.js#L49-L55

The following might work:

test('error.code is defined on failure if applicable', async t => {
	const {code} = await t.throwsAsync(execa(''));
	t.is(code, 'ERR_INVALID_ARG_VALUE');
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, using an empty command on Windows doesn't generate ERR_INVALID_ARG_VALUE. There aren't many errors thrown consistently across both POSIX & Windows and also across multiple Node versions. The one I found that seems to work is giving an incorrect type for cwd. So I used that again here in the updated version.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works 👍

Copy link
Collaborator

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mterrel!

When it comes to detecting ENOENT by looking at the error message, I do agree that it might not be right to make that logic work only for users with English locales. Not only wouldn't it be fair to non-English speakers, but it might confuse users would get very different behaviors between machines and might not realize this is due to the locale.

@mterrel mterrel force-pushed the remove-enoent-windows branch from dcde090 to 469d76b Compare December 1, 2020 17:58
@mterrel mterrel requested a review from ehmicky December 1, 2020 18:03
Copy link
Collaborator

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good to me. Thanks @mterrel!
@sindresorhus What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect ENOENT error on Windows with shell=true when child exits with code 1
3 participants