Feature add custom http headers #764
Conversation
| let httpHeaders = {}; | ||
| // prefer --headersObject if specified | ||
| if (cli.flags.headersObject) { | ||
| httpHeaders = JSON.parse(cli.flags.headersObject); // note: this will generate a recognizable error for the user to act on |
There was a problem hiding this comment.
Minor change here: we had a getCLISchemaHeadersJSON() function that did a lot of setup around JSON.parse(), and in the end obscured the error from the user. Instead, I’d like to expose the error to the user so it’s more clear what’s not parsing correctly.
| // otherwise, parse --header | ||
| else if (Array.isArray(cli.flags.header)) { | ||
| cli.flags.header.forEach((header) => { | ||
| const firstColon = header.indexOf(":"); |
There was a problem hiding this comment.
This was using a RegEx instead (/(\w+)(\:)\s((\w+)[\-|\:]?.+)/gi;), but in testing I found it to be brittle and a little awkward. Instead I opted for more of a curl-like approach where the first : designates the key–value relationship. It’s then up to the user to format their header correctly.
Codecov Report
@@ Coverage Diff @@
## main #764 +/- ##
==========================================
- Coverage 96.34% 94.12% -2.22%
==========================================
Files 13 13
Lines 547 562 +15
Branches 194 195 +1
==========================================
+ Hits 527 529 +2
- Misses 19 32 +13
Partials 1 1
Continue to review full report at Codecov.
|
drwpow
force-pushed
the
feature-add-custom-http-headers
branch
from
4456647 to
4d9d18c
Compare
| * or Accept: text/yaml to be sent in order to figure out how to properly fetch the OpenAPI/Swagger document as code. | ||
| * These headers will only be sent in the case that the schema URL protocol is of type http or https. | ||
| */ | ||
| httpHeaders?: Headers; |
There was a problem hiding this comment.
This is an opinionated change, but in the previous PR we allowed httpHeaders?: to be a polymorphic type, from null to a JS object to a Map. We then had to implement quite a bit of runtime logic to check to make sure everything lined up correctly.
Instead, I opted to go with just a plain JS object for headers because that’s what gets passed to fetch() anyway. Servers are lax in what they accept. And while I completely get the author’s original desire to strongly type their request headers, I believe that’s outside the scope of this library, and could only introduce possible bugs/unnecessary maintenance.
| * | ||
| * @default {string} GET | ||
| */ | ||
| httpMethod?: string; |
There was a problem hiding this comment.
Similar approach: rather than maintain a discriminated union of all valid HTTP methods, I opted instead to allow a simple string like most every other library allows. Perhaps your server allows more!
Again, I know this was not the author’s original intent, and I respect their level of quality! But I feel this was one of many details which just added on a lot of maintenance and potential typing bugs without much benefit to the average user.
Supercedes #713.
Changes
Other than the commented code below, the following changes were made from #713
loadunit test was removed. Reason being that didn’t give much assurance that the entire CLI was behaving as desired (in my limited testing, it wasn’t)util.test.tsunit test was also removed. This was a little too implementation detail-y, and it only retested what existed in other tests.