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

Simplify request wrapper #218

Merged
merged 5 commits into from Feb 24, 2021
Merged

Simplify request wrapper #218

merged 5 commits into from Feb 24, 2021

Conversation

davidpatrick
Copy link
Contributor

Description

The goal of this PR is to expand the support for users of this library to fully customize the underlying request, while simplifying the default request options.

Adds

  • options.fetcher - this is a promise returning function that fetches the jwks data. We want to make sure that users can fully customize how data is fetched, this option will allow for a user to replace the underlying request logic and supply their own data fetching logic.
  • options.requestAgent - previously we gated the options that could be passed into a requestAgent, which required we have some type of frankenstein solution for supporting proxies. This will allow the user supply their own agent in the request and thus allowing them to support the proxy at this level.

Removes

  • options.requestAgentOptions - now with requestAgent this will no longer be needed
  • options.proxy - now with requestAgent a user can control their proxy flow, and with the fetcher option a user can completely control the request flow
  • axios - we have had a lot of issues pop up after moving from the EOL request library to axios. With the fetcher option, a user can now use their own request library, and this PR introduces using the node internals to make the requests.

src/wrappers/request.js Outdated Show resolved Hide resolved
@davidpatrick davidpatrick force-pushed the new-request-options branch 4 times, most recently from 0c8b0ed to 6460545 Compare February 19, 2021 23:36
Copy link
Contributor

@adamjmcgrath adamjmcgrath left a comment

Choose a reason for hiding this comment

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

lgtm - couple of suggestions

package.json Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
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.

None yet

3 participants