-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SDK-3105] Add httpTimeoutInSeconds to control fetch timeout #875
Conversation
f1248e2
to
8b751d5
Compare
@@ -1194,7 +1204,8 @@ export default class Auth0Client { | |||
redirect_uri, | |||
...(timeout && { timeout }), | |||
auth0Client: this.options.auth0Client, | |||
useFormData: this.options.useFormData | |||
useFormData: this.options.useFormData, | |||
timeout: this.httpTimeoutMs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're overriding the timeout
in 1205
Not sure if anyone is doing getTokenSilently({ timeout: ... })
since it's not documented, but we should probably keep that working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot, was just an oversight.
I think it should still resolve to the same thing but agree it's a bit of a smell.
src/Auth0Client.ts
Outdated
@@ -1106,7 +1115,8 @@ export default class Auth0Client { | |||
grant_type: 'authorization_code', | |||
redirect_uri: params.redirect_uri, | |||
auth0Client: this.options.auth0Client, | |||
useFormData: this.options.useFormData | |||
useFormData: this.options.useFormData, | |||
timeout: this.httpTimeoutMs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, this is the place where I thought we might break getTokenSilently({ timeout: ... })
- not sure if anyone is using that?
Maybe we should do: timeout: customOptions.timeout || this.httpTimeoutMs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, stepping back a bit the API does look a bit odd here.
With this PR it's now possible to construct a client like this:
const client = new Auth0Client({
domain,
clientId,
httpTimeoutInSeconds: 60,
authorizeTimeoutInSeconds: 60
});
// Then call getTokenSilently like this.
// `timeoutInSeconds` only affects the timeout on /authorize but no way to configure http timeout here
await client.getTokenSilently({ timeoutInSeconds: 30 });
Should we also add this config in to override at the getTokenSilently
level, or do you think just explaining the behaviour of timeoutInSeconds
in the docs is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless, this:
Maybe we should do: timeout: customOptions.timeout || this.httpTimeoutMs?
customOptions.timeout
here is documented as being "A maximum number of seconds to wait before declaring the background /authorize call as failed for timeout", so the two timeouts relate to different things. But would be confusing as I mentioned above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
customOptions.timeout
isn't documented anywhere, that's the docs for timeoutInSeconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, if I call auth0.getTokenSilently({ timeout: 100 })
the HTTP Timeout for the code exchange will be 100ms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, sorry - I think I see what you're saying. You're covering the case where someone has inadvertently used getTokenSilently({ timeout: 100 });
instead of the documented getTokenSilently({ timeoutInSeconds: 100 });
, which they could well do because we allow arbitrary parameters?
It would be easy to cover off that use case, sure - just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this now 👍🏻 was a good spot, thanks
9d22b5c
to
130a6b8
Compare
Changes
This PR adds a new client configuration option
httpTimeoutInSeconds
that allows consumers to configure the fetch timeout when calling theoauth/token
endpoint. The default remains at 10 seconds if not specified.References
Fixes #871
Testing
Checklist