Skip to content

Support AbortController#490

Merged
sindresorhus merged 21 commits intosindresorhus:mainfrom
jopemachine:support-abort-controller
Feb 13, 2022
Merged

Support AbortController#490
sindresorhus merged 21 commits intosindresorhus:mainfrom
jopemachine:support-abort-controller

Conversation

@jopemachine
Copy link
Contributor

Fixes #449.

I'd appreciate it if you could let me know if I need to write more tests or documents.

Thanks for all your efforts for this awesome library :)

@ehmicky
Copy link
Collaborator

ehmicky commented Feb 4, 2022

Thanks a lot for this contribution @jopemachine!

We might want to name this option signal instead of abortSignal to match the name currently used by child_process.spawn().

More fundamentally, I am wondering whether the core signal option should just be used instead? I have written some potential ideas in the issue, please let me know what you think.

@jopemachine
Copy link
Contributor Author

We might want to name this option signal instead of abortSignal to match the name currently used by child_process.spawn().

Thanks for pointing it out, I just updated the option.

@jopemachine
Copy link
Contributor Author

Should I delete this document too? or should I mark the cancel method as deprecated?

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.

I believe we can directly delete that section of the documentation, good catch! 👍

Thanks again for this PR @jopemachine! There are a couple of things I have noted.
The tests look great! (But the CI tests are currently failing)

@jopemachine
Copy link
Contributor Author

I believe we can directly delete that section of the documentation, good catch! 👍

Thanks again for this PR @jopemachine! There are a couple of things I have noted. The tests look great! (But the CI tests are currently failing)

I appreciate your detailed feedback :)
I tried to reflect on all the feedback as much as I can.
If I'm missing something or got something wrong, I appreciate your letting me know.

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 looks good to me, thanks a lot @jopemachine!

Now waiting for another review from @sindresorhus.

@ehmicky ehmicky requested a review from sindresorhus February 8, 2022 14:06
@sindresorhus sindresorhus merged commit c6e791a into sindresorhus:main Feb 13, 2022
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.

Support AbortController

3 participants