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

Make error.killed more consistent #248

Merged
merged 4 commits into from
May 14, 2019
Merged

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented May 14, 2019

error.killed is always true or false with execa().

But with execa.sync(), it can also be undefined sometimes, e.g. on ENOENT. This PR ensures error.killed is always a boolean.

Details:

  • spawn() returns a childProcess with childProcess.killed
  • spawnSync() does not return this, but we can emulate most of the cases by checking the signal property.

Tests are added for this as well.

test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Unrelated to this PR, but what do you think about renaming .killed to .isKilled for naming consistency?

@sindresorhus
Copy link
Owner

spawnSync() does not return this, but we can emulate most of the cases by checking the signal property.

It's weird that the synchronous version has this inconsistency. Should we open a Node.js issue about it?

@ehmicky
Copy link
Collaborator Author

ehmicky commented May 14, 2019

It's weird that the synchronous version has this inconsistency. Should we open a Node.js issue about it?

That's because spawn() return a ChildProcess instance, while spawnSync() returns a plain object with fewer/different properties. It's not only killed. I think Node.js would say it's a design decision.

Unrelated to this PR, but what do you think about renaming .killed to .isKilled for naming consistency?

isKilled is more consistent with isCanceled. However spawned.killed already exists in core Node.js, so I would think using property names users are already expecting or familiar with is more important.

@sindresorhus
Copy link
Owner

isKilled is more consistent with isCanceled. However spawned.killed already exists in core Node.js, so I would think using property names users are already expecting or familiar with is more important.

You could argue that about .code vs exitCode though.

@ehmicky
Copy link
Collaborator Author

ehmicky commented May 14, 2019

Yes I thought of that as well. The thing is .code is actually different from exitCode because they don't have the same type. Whereas .killed is almost identical both in type and behavior.

We can rename exitCode to code though if you'd prefer (and exitCodeName to codeName).

@sindresorhus
Copy link
Owner

Good point. Nah, we can keep it as-is.

@sindresorhus sindresorhus merged commit fd064a7 into master May 14, 2019
@sindresorhus sindresorhus deleted the feature/consistent-killed branch May 14, 2019 17:24
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.

2 participants