redis-js
redis-js copied to clipboard
upstash
feat: Added a timeout option instead of signal in HttpClientConfig
In most use cases, the AbortController should be new for each fetch request and should not be stored as a HttpClientConfig.
ref: https://github.com/upstash/upstash-redis/issues/778#issuecomment-1861059008
Hey, we can't simply remove a feature from the SDK; we always have to consider backward compatibility. Also, since that feature isn't really requested and I don't see the benefits compared to a manual abort, I prefer not to move forward with this.
@ogzhanolguncu Thanks for taking a look at the PR 🙏
Hey, we can't simply remove a feature from the SDK; we always have to consider backward compatibility.
I'm sorry, exactly...😓 but it didn't seem like a good use case for the client to have singal, and it actually confused me.
We had upstash/redis connectivity issues the other day and the application crashed. (We use upstash through vercel/kv)
I noticed that I could usesignal option, but found that it could only be set at the time I initialized the client, not per fetch requested.
Is there any good solution?
Initially, we wanted to introduce command-level abort, but it seemed like a lot of work. We opted for client-level abort because client instantiation is cheap. However, if we were to add anything, it should definitely be command-level abort to have total control over operations.
thx reply :) ok, I'll try to implements the "command-level abort" interface💪
@ogzhanolguncu I gave up, sorry... I found it quite difficult to add options to the position argument-based interface. This is because arguments cannot be added after variable-length arguments.
I think the fundamental solution would be to change the argument to an object, like a named argument. However, this would require a major update due to the breaking changes.
As a provisional solution, how about keeping signal for backward compatibility, but adding additional clientOptions such as timeout and signalFactory?