feat: Add HTTP header configurations to options #379
Conversation
| uploadBatchSize: 100, | ||
| headers: { | ||
| 'Content-Type': 'application/x-www-form-urlencoded; charset=UTF-8', | ||
| }, |
There was a problem hiding this comment.
Since this option is a nested object, will a user passing in their own headers completely overwrite this (and therefore delete this Content-Type value)? Or will it keep Content-Type and append new headers from there?
There was a problem hiding this comment.
Also, don't forget to put a comment for headers at the description of all the possible options at the top of this file
There was a problem hiding this comment.
Added docs explaining this
|
Left two small comments, otherwise looks good |
There was a problem hiding this comment.
I don't know if this is enough - in our option validation, I feel like _parseConfig will remove any keys that aren't in the default options (e.g. any one that's not Content-Type) - could you confirm this + add in the unit test cases a completely new header - perhaps (Authorization: Bearer NOT_A_REAL_BEARER_TOKEN)?
|
@kelvin-lu Updated code to account for freeform properties like |
# [8.2.0](v8.1.0...v8.2.0) (2021-04-28) ### Features * Add HTTP header configurations to options ([#379](#379)) ([4aaa26f](4aaa26f))
| } | ||
|
|
||
| // Add exception in headers | ||
| const freeFormObjectKeys = new Set(['headers']); |
There was a problem hiding this comment.
nit - this belongs better in the options.js?
| for (var key in config) { | ||
| if (Object.prototype.hasOwnProperty.call(config, key)) { | ||
| if (freeFormObjectKeys.has(key)) { | ||
| options[key] = { ...options[key], ...config[key] }; |
There was a problem hiding this comment.
I think you might want to add a TODO to find a better soln here - this bypasses all checks, right?
There are still probably some checks to this - as in, header values can't be functions or nested objects, right?
3 participants
Summary
headerstooptionsoptions.headersChecklist