Migrate from Request #134
Closed
Migrate from Request #134
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidpatrick
force-pushed
the
request-deprecation
branch
from
41020bf
to
42550d5
Compare
adamjmcgrath
requested review from
adamjmcgrath
and removed request for
adamjmcgrath
adamjmcgrath
requested changes
Mar 19, 2020
src/JwksClient.js
Outdated
| ...this.options.httpOptions | ||
| }) | ||
| .then(res => { | ||
| if (res.statusCode < 200 || res.statusCode >= 300) { |
There was a problem hiding this comment.
By default axios rejects non 2xx status codes, using the validateStatus option. So this code path will never be hit (unless the user overriders validateStatus)
src/JwksClient.js
Outdated
| if (res) { | ||
| return cb(new JwksError(res.body && (res.body.message || res.body) || res.statusMessage || `Http Error ${res.statusCode}`)); | ||
|
|
||
| return axios.request({ |
There was a problem hiding this comment.
I would favour removing the return statement, just because it might confuse
tests/jwksClient.tests.js
Outdated
|
|
||
| const client = new JwksClient({ | ||
| jwksUri: `${jwksHost}/.well-known/jwks.json` | ||
| }); | ||
|
|
||
| client.getKeys(err => { | ||
| expect(err).not.to.be.null; | ||
| expect(err.message).to.equal('Unknown Server Error'); | ||
| expect(err.message).to.equal('Request failed with status code 500'); |
There was a problem hiding this comment.
You could add an assertion that error is of type JwksError
There was a problem hiding this comment.
Adding the JwksError expectation, correctly exposes this issue #134 (comment)
davidpatrick
force-pushed
the
request-deprecation
branch
from
42550d5
to
99d2c68
Compare
|
Closing this PR in favor of opening a new one where we will replace request library without any breaking changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In response to the Request Deprecation request/request#3142 we have done our own assessment of the situation and potential replacements.
Requirements
We looked into our use of Request and found that we wanted to continue to support the current 5 features we use with request:
Additionally, we have had requests for further use of the request library. Because of this, we want to expose the config of the under pinning http library we choose.
Research
We looked into using the following
Axios is the only library that supports all 5 features out of the box. Got and node-fetch met all requirements, however required workaround or external library for using a proxy. After testing each option, Axios required the least amount of configuration on this libraries end.
Migration
To expose the under pinning library we use to make requests, we will be changing the API for controlling the http agent, headers, proxy, and exposing a new option
httpOptionsthat will passthrough to Axios https://github.com/axios/axios/tree/v0.19.2#request-configFixes issue #126