fix: add echo requester and fix timeouts #14
Conversation
| import { Response } from './types'; | ||
|
|
||
| export function isNetworkError({ isTimedOut, status }: Omit<Response, 'content'>): boolean { | ||
| return !isTimedOut && ~~status === 0; |
There was a problem hiding this comment.
I'm not sure I get the ~~status === 0 part of this check? Do we get a status of 0 back when there's a network error?
There was a problem hiding this comment.
I think it's when there's no response at all
There was a problem hiding this comment.
I copied it from the existing client, I don't know why there is the ~~ but yes, here it indicates an error.
There was a problem hiding this comment.
Okay, thanks for clarifying! The ~~ ensures we're always working with an integer (even if status is a string etc), without impacting performance too much.
There was a problem hiding this comment.
Changes looks good on my side!
Only thing is that we should start documenting:
- the changes done between the current client implementation and the new clients
- what should be manually added in a client to make it work like the JS one
It will help us implement future clients and also simplify the work if we need external contributors
| import { Response } from './types'; | ||
|
|
||
| export function isNetworkError({ isTimedOut, status }: Omit<Response, 'content'>): boolean { | ||
| return !isTimedOut && ~~status === 0; |
There was a problem hiding this comment.
I think it's when there's no response at all
|
(PR title is |
| @@ -32,7 +32,7 @@ export class SearchApi { | |||
|
|
|||
| protected interceptors: Interceptor[] = []; | |||
|
|
|||
| constructor(appId: string, apiKey: string) { | |||
| constructor(appId: string, apiKey: string, requester?: Requester) { | |||
| this.setApiKey(SearchApiApiKeys.appId, appId); | |||
| this.setApiKey(SearchApiApiKeys.apiKey, apiKey); | |||
| this.transporter = new Transporter({ | |||
There was a problem hiding this comment.
This is not in the scope of this PR, but just wanted to make sure everyone's aware of this; I would recommend making the hosts (or the whole transporter) something you can pass to the constructor as well. Once Metis is released, the client would need to work with different hosts than the one we set by default.
There was a problem hiding this comment.
Do you think we should prepare this codebase for Metis even if we don't have an ETA?
There was a problem hiding this comment.
(Does not cost anything, but just wondering)
There was a problem hiding this comment.
It's good to keep in mind for sure, otherwise we might have to rework a lot later on 🙂 Besides the hosts part, not a lot should change, though (perhaps the retry strategy but that is a problem for later 😄 )
There was a problem hiding this comment.
Good point I'll include the host too
|
🔨 The codegen job will run at the end of the CI. _Make sure your last commit does not contains generated code, it will be automatically pushed by our CI. |
|
oops |
* fix: simplify model with types and simplify api (#15)
Add possibility to change requester, and include a basic echo requester that sends the request back, used for the CTS.
Also fixes the read/write timeouts, the read/write hosts, and make the flow a little bit clearer.