Skip to content
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

Added timeout with default value of 30s #132

Merged
merged 6 commits into from
Apr 11, 2020
Merged

Conversation

Cooke
Copy link
Contributor

@Cooke Cooke commented Feb 19, 2020

Description

The purpose of this pull request is to add a timeout (with default value) which applies to the request for fetching keys.

The introduction of the timeout setting isn't itself breaking but the default value of 30s could be considered breaking, though it should be a sane value that everyone could live with.

References

Issue #131

Testing

No tests have been added.

This change depends on the timeout functionality of the request lib.

Manual verification has been done and auto tests have been executed with npm test.

Checklist

  • [ yes ] I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • [ yes ] All active GitHub checks for tests, formatting, and security are passing
  • [ not sure ] The correct base branch is being used, if not master

@Cooke Cooke requested a review from a team February 19, 2020 08:56
@davidpatrick
Copy link
Contributor

Hey @Cooke thanks for the contribution. We are currently migrating off of request because of the libraries deprecation. We will be moving to axios and exposing the axios config options which will allow you to set the timeout. #134

We can move forward with this PR, but its use would break in PR #134. Let me know if you are able to wait.

Thanks

@Cooke
Copy link
Contributor Author

Cooke commented Mar 23, 2020

@davidpatrick we can wait. We are already using our own fork in the meanwhile. Thank you

@apumb
Copy link

apumb commented Apr 2, 2020

Hi @davidpatrick, Just wondering if there was a timeframe for moving to axios? This will solve some issues we're facing but the timing will give us a better idea of how we should proceed right now.
Thanks

@davidpatrick
Copy link
Contributor

@apumb working on getting #135 merged asap

@Cooke do you want to update this PR to include the new src/wrappers/request.js https://github.com/auth0/node-jwks-rsa/pull/135/files#diff-c51c2f09575f5eff8f9d311bf261a763

We will need to pass the timeout option through to this new wrapper

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@davidpatrick davidpatrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

@Cooke
Copy link
Contributor Author

Cooke commented Apr 6, 2020

@davidpatrick that should hopefully do it. But you should better check it. Also I think I screwed up the ticket references, sorry.

@apumb
Copy link

apumb commented Apr 6, 2020

Thanks @davidpatrick, this is awesome!!

@davidpatrick
Copy link
Contributor

@Cooke I've merged in the request dep changes, can you rebase this with master? That should resolve the conflicts

@davidpatrick davidpatrick added this to the v1-Next milestone Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants