feat: add signal support and fix timeout, closes #5 #68
Conversation
src/client.js
Outdated
| @@ -82,7 +83,7 @@ class MistralClient { | |||
| 'Authorization': `Bearer ${this.apiKey}`, | |||
| }, | |||
| body: method !== 'get' ? JSON.stringify(request) : null, | |||
| timeout: this.timeout * 1000, | |||
| signal: signal ?? AbortSignal.timeout(this.timeout * 1000), | |||
There was a problem hiding this comment.
Due to the limitations of what's possible when attempting to merge a user provided signal and a timeout signal, this simple approach may be the best that can be done without passing in an AbortController and implementing a timeout that calls its abort method. Thus if the user provides a signal, they are responsible for managing timeouts as well. This would be non-obvious to most users though. On the flip side, passing around an AbortController seems non-standard.
We could document the parameter with something along these lines:
"Users must manage timeouts for their own AbortSignals. Without a user-provided signal, a default timeout of [x] seconds applies automatically."
There was a problem hiding this comment.
Actually, that may not be the case re: merging, as the signal.reason /is/ available and should be possible to transfer to a new merged signal
(
LLM: says no,
ME: waiiiit, didn't I check the reason in a catch block ..."
)
|
Let's go! |
src/client.js
Outdated
| @@ -221,6 +246,8 @@ class MistralClient { | |||
| * @param {*} safePrompt whether to use safe mode, e.g. true | |||
| * @param {*} toolChoice the tool to use, e.g. 'auto' | |||
| * @param {*} responseFormat the format of the response, e.g. 'json_format' | |||
| * @param {*} [signal] - optional AbortSignal instance to control request | |||
There was a problem hiding this comment.
Actually, it might be worth considering whether to move signal into a second object param, and keep it out of the bag of data that is sent to the server. It's a bit cleaner that way.
On a related note, I realised that the JSDoc comments are actually incorrect (see)
combineSignalsinsrc/client.jsto merge AbortSignal instances._requestmethod insrc/client.jsto accept an additional argumentsignaland use thecombineSignalsfunction to merge with a default timeout signal.chatandchatStreammethods insrc/client.jsto use a single objectdatafor the main chat configuration and an additional options objectoptions, where the signal is passed.