search

connor4312 / cockatiel

🐦 A resilience and fault-handling library. Supports Backoffs, Retries, Circuit Breakers, Timeouts, Bulkhead Isolation, and Fallbacks.
MIT License
1.59k stars 50 forks source link

Remove the neverAbortedSignal #94

Open ghost91- opened 4 months ago

ghost91- commented 4 months ago

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.

connor4312 commented 4 months ago

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- commented 4 months ago

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.