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

AbortSignal support #69

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

BerkliumBirb
Copy link

No description provided.

mdlavin
mdlavin previously approved these changes Oct 16, 2024
@mdlavin
Copy link
Member

mdlavin commented Oct 16, 2024

@BerkliumBirb thank you for the improvement and especially thank you for the testcase and doc updates too! Due to some security requirements we have, all our projects require signed commits. Do you mind re-pushing your commits with signatures? If you don't want to do that, I can re-create your changes and merge. The downside is that you won't get "credit" for the commits.

@mdlavin
Copy link
Member

mdlavin commented Oct 16, 2024

@BerkliumBirb P.S. after I approved the automation run, I see that it failed with a missing reference

README.md Outdated Show resolved Hide resolved
Wasn't able to find some reputable solution from core-js for example. Wasn't able to pass node arguments through nyc too.
@BerkliumBirb
Copy link
Author

  • Signed commits with GPG signature
  • Added abortcontroller-polyfill, because I failed to find the way to pass --experimental-abortcontroller to Node for testing or find any more reputable polyfill from core-js for example.
  • Fixed doc entry

@mdlavin
Copy link
Member

mdlavin commented Nov 27, 2024

I appreciate the time you put into this. The need to add a polyfill for tests concerns me a little bit.

Does the addition of AbortSignal bump up the Node.js requirements to >= v16? If so, we should mark the change as a breaking change. I can push a new commit to this branch to do that.

@BerkliumBirb
Copy link
Author

BerkliumBirb commented Nov 28, 2024

AbortController is marked as implemented from v14.17.0. In v14.17.0+ it can be used under experimental flag. I tried to pass experimental flag through nyc to not bring any polyfills, but wasn't able to.

This polyfill is only used in tests, and the actual code doesn't use anything global. We can ditch the whole AbortController/AbortSignal stuff and use { aborted: boolean } as AbortSignal in tests instead and remove any polyfills, but I don't know if it's a good idea to trim API down to the only used field.

I also don't really think this feature deserves a major bump, but if you think it does, we can go that way.

So, we have 4 options:

  1. Try to pass --experimental-abortcontroller through nyc. I need help with this one.
  2. Use dummy { aborted: boolean } as AbortSignal instead of the actual Controller/Signal.
  3. Bump major to require node v16
  4. Use polyfill

Which one should we go?

@BerkliumBirb BerkliumBirb requested a review from mdlavin December 7, 2024 00:37
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.

3 participants