Skip to content

Remove the neverAbortedSignal#94

Open
ghost91- wants to merge 1 commit intoconnor4312:masterfrom
ghost91-:remove-nerver-aborted-signal
Open

Remove the neverAbortedSignal#94
ghost91- wants to merge 1 commit intoconnor4312:masterfrom
ghost91-:remove-nerver-aborted-signal

Conversation

@ghost91-
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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