feat!: Request Revamp
#1938
feat!: Request Revamp
#1938
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
`Headers` in Node v18 are not case-insensitive
…brary-nodejs into request-revamp
|
Warning: This pull request is touching the following templated files:
|
product-auto-label
bot
added
size: xl
product-auto-label
bot
added
size: l
| @@ -60,15 +49,15 @@ interface GenerateAuthHeaderMapOptions { | |||
| // The AWS service URL query string. | |||
| canonicalQuerystring: string; | |||
| // The HTTP method used to call this API. | |||
| method: HttpMethod; | |||
| method: string; | |||
There was a problem hiding this comment.
why is this type changing? seems more flexible? also not clear if this change has to do with the gaxios upgrade.
There was a problem hiding this comment.
The HttpMethod was a handwritten version for what the spec provides. The spec accepts any method as a string.
| if (typeof requestPayload !== 'string' && requestPayload !== undefined) { | ||
| throw new TypeError( | ||
| `'requestPayload' is expected to be a string if provided. Got: ${requestPayload}` | ||
| ); |
There was a problem hiding this comment.
do we know what requestPayload type could be? This could be an ugly error Got [object Object]
There was a problem hiding this comment.
We want it to be a string, however we marshal objects to strings a few lines above it. This is preserving the existing functionality and makes it clearer, we used to throw later in the call stack.
| } | ||
| opts.headers = Gaxios.mergeHeaders(opts.headers); | ||
|
|
||
| this.addUserProjectAndAuthHeaders(opts.headers, r.headers); |
| } | ||
| } else { | ||
|
|
||
| if (!METHODS_SUPPORTING_REQUEST_BODY.includes(method)) { |
There was a problem hiding this comment.
I see why you made this change - definitely more readable! - but ideally could be another PR (given this is a big PR already, and this is more of a refactor, and we're already doing so many other gaxios changes). NB.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| delete (values as {[index: string]: any})[key]; | ||
|
|
||
| // Keep defined fields. |
There was a problem hiding this comment.
Again, another small refactor that in general seems harmless but in the context of a feature, might be better as a F/U.
There was a problem hiding this comment.
Understood, however this required the refactor now given the more precise type requirements of
URLSearchParams.
Description
This is a large refactor of the auth library's internals with sweeping improvements in UX, correctness, performance, security, and scalability.
Examples:
fetchAPI conventionsresponseTypeparameter fromGaxiosOptions(which was problematic in error cases)HeadersTesting
All existing tests pass and enhanced numerous throughout the codebase. Notably, the sample tests required minimal changes.
Impact
This revamp brings number of improvements to library:
Headers, while allowing the open, flexibleHeadersInitfor requests.RequestInitbody types, users can omit certain headers, such ascontent-typeRequest,Headers,URLSearchParams, etc.)/Additional Information
While a large PR, most of the changes are internal and doesn't affect external API consumption much outside from returning
Headers.Note: a large portion of this PR are non-functional changes related to Node 18's
Headers- which are not case-insensitive when compared in tests (which required a lot of lower-casing header names).A follow-up PR will add
fetchsupport.Fixes: #1829
🦕