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 the neverAbortedSignal #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghost91-
Copy link
Contributor

This PR removes the neverAbortedSignal and instead simply does not pass a signal by default. The behavior is essentially the same, but it simplifies the code a bit, in my opinion.

Unfortunately, this is technically a breaking change because consumers might rely on always receiving a signal. So I'm not sure whether this is actually worth doing, but I wanted to bring it up nonetheless. For example, if a major release is planned anyways for other reasons, it might make sense to slip this in.

BREAKING CHANGE: The signal in IDefaultPolicyContext is now optional. The case
when it's not available should be handled like a signal that is never aborted.
@connor4312
Copy link
Owner

I'm not a very big fan of this. I see that it's useful for consumers to save a little bit of work if they don't know whether an abort signal will get passed and can avoid adding listeners to the neverAbortedSignal, but this is a pretty marginal performance difference and makes other usability harder because now all code must check whether the signal is passed before they can use it.

@ghost91-
Copy link
Contributor Author

ghost91- commented Jul 22, 2024

Fair point. I'm very used to dealing with optional things by using optional chaining and typically, the signal is optional anyways where you pass it into (fetch, axios, etc.), so I didn't think of it as a big deal (aside from it being a breaking change).

To be honest, the performance improvement isn't even the main argument for me, it's that the code becomes a bit easier to understand, in my opinion. But maybe we disagree on that. Feel free to close this if you think we should not do it.

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