Fix issue with empty headers #37
Conversation
Codecov Report
@@ Coverage Diff @@
## master #37 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 43 43
Branches 18 18
=========================================
Hits 43 43
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hi @bkniffler thanks for the contribution! Can you check my review comment? This would fix #35 if it is consitent with the browsers fetch api and node-fetch.
src/main.js
Outdated
| @@ -42,7 +42,7 @@ export const isInstanceOfHeaders = (val) => { | |||
| * @property {String} params - The header params as string | |||
| */ | |||
|
|
|||
| const getHeaderString = (name, val) => ` -H "${name}: ${val.replace(/(\\|")/g, '\\$1')}"`; | |||
| const getHeaderString = (name, val) => ` -H "${name}: ${(val || '').replace(/(\\|")/g, '\\$1')}"`; | |||
There was a problem hiding this comment.
I'd probably use `${val}`.replace instead of (val || '').replace to be consistent with the implementation of node-fetch and the browsers fetch api.
There was a problem hiding this comment.
Interesting, thanks for linking the issue, wasn't aware of browser implementation. I'll change it right away!
Be more consistent with browser fetch when handling undefined/null headers
If for some reason a header value is undefined/null, we get an error (
can't call function replace of undefined). We could filter out empty headers also, but if the header is there and empty, maybe we'd still want it output?