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

Simplify cancel() method #226

Merged
merged 1 commit into from
May 10, 2019
Merged

Simplify cancel() method #226

merged 1 commit into from
May 10, 2019

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented May 9, 2019

This is a follow up on #189 by @ammarbinfaisal. It simplifies the cancel() method by removing some code, without changing behavior.

@ehmicky ehmicky requested a review from sindresorhus May 9, 2019 19:11
@sindresorhus
Copy link
Owner

Are you sure these checks are not needed? Could be that we're just not having tests for their behavior.

@ehmicky
Copy link
Collaborator Author

ehmicky commented May 10, 2019

Yes I should have provided some background information on the changes:

  • isCanceled is always false because if cancel() was called, then result.signal will be defined.
  • spawned.kill() is a fast noop (does not throw and returns false) if the child process has been killed. I.e. checking if (spawned.killed) {} before is not needed.

@sindresorhus sindresorhus merged commit 2438df2 into master May 10, 2019
@sindresorhus sindresorhus deleted the feature/simplify-cancel branch May 10, 2019 10:51
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