Add withAbortSignal() #93
Conversation
src/DefaultRESTClient.ts
Outdated
| @@ -183,6 +183,8 @@ export default class DefaultRESTClient<RT, ERT> implements IRESTClient<RT, ERT> | |||
| body: this.body as BodyInit, | |||
| // @ts-ignore (Credentials are not supported on NodeJS) | |||
| credentials: this.credentials, | |||
| // 60-second timeout expressed in ms | |||
| timeout: 60000, | |||
There was a problem hiding this comment.
Any reason not to use signal: AbortSignal.timeout( 60000 ) instead, since the options doc says Signal is recommended instead.
There was a problem hiding this comment.
If we want to use a timeout, the timeout value should be set on the class and referenced just like we do for credentials.
So I would suggest we :
- add a new field for timeout, we can default to 60 seconds if we want.
- add new
with*method so this can be set using the builder pattern.
Agree with @mooreds - sounds like the API doc suggest using Signal instead.
Then, to complete this work, we'll need to expose this in the FusionAuth REST client. Perhaps we can add this to the builder once this PR is complete.
There was a problem hiding this comment.
I've updated the PR to add a withAbortSignal() instead.
I'm not sure how this gets incorporated into the two files mentioned above, though.
| @@ -5,7 +5,8 @@ | |||
| "declaration": true, | |||
| "sourceMap": true, | |||
| "lib": [ | |||
| "es2015" | |||
| "es2015", | |||
| "dom" | |||
There was a problem hiding this comment.
required to add the AbortSignal type
Addresses #92