Skip to content

Fix the getHeaders method signatures#434

Merged
mapleeit merged 3 commits intoform-data:masterfrom
ScriptSmith:master
Sep 3, 2019
Merged

Fix the getHeaders method signatures#434
mapleeit merged 3 commits intoform-data:masterfrom
ScriptSmith:master

Conversation

@ScriptSmith
Copy link
Copy Markdown
Contributor

@ScriptSmith ScriptSmith commented Jul 27, 2019 •
edited
Loading

In the readme, getHeaders is described as accepting and returning an array:

Array getHeaders( [Array userHeaders] )

The function doesn't return an array, and while it can technically accept one, the output it produces doesn't seem useful:

form-data/lib/form_data.js

Lines 293 to 306 in 905f173

FormData.prototype.getHeaders = function(userHeaders) {
var header;
var formHeaders = {
'content-type': 'multipart/form-data; boundary=' + this.getBoundary()
};
for (header in userHeaders) {
if (userHeaders.hasOwnProperty(header)) {
formHeaders[header.toLowerCase()] = userHeaders[header];
}
}
return formHeaders;
};

Here's how it's defined in index.d.ds:

form-data/index.d.ts

Line 14 in 905f173

getHeaders(): FormData.Headers;

form-data/index.d.ts

Lines 26 to 29 in 905f173

declare namespace FormData {
interface Headers {
[key: string]: any;
}

This is more correct, but it says that getHeaders doesn't take any arguments, which is incorrect.

I've updated the readme to say that it takes and returns Headers as defined in index.d.ts (but perhaps it should be something more generic like Object). I've also updated the type definitions to include the userHeaders parameter.

Furthermore, line 300 in getHeaders seems redundant:

form-data/lib/form_data.js

Line 300 in 905f173

if (userHeaders.hasOwnProperty(header)) {

It was added in ebf47ef because of an issue identified in codacity? / codacy. I'm not sure what problem this could be mitigating.

If it is redundant I'll add another commit to correct it.

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 27, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 98.122% when pulling 2b4e478 on ScriptSmith:master into 5c6bf52 on form-data:master.

@mapleeit
Copy link
Copy Markdown
Collaborator

mapleeit commented Aug 30, 2019 •
edited
Loading

Hi @ScriptSmith Thank you for your patch and sorry for the late reply. (I missed this)

I'm not sure the type of input param is just

interface Headers {
    [key: string]: any;
  }

I guess it may be NodeJS.IncomingHttpHeaders. Otherwise the hasOwnProperty is redundant.

@alexindigo Do you still remember the reason why adding hasOwnProperty? : ） ebf47ef#diff-121026c01234c71e2b3b316f7abc5afcR217-R230

@ScriptSmith
Copy link
Copy Markdown
Contributor Author

ScriptSmith commented Aug 31, 2019

Good point, I think OutgoingHttpHeaders would be more appropriate given that form-data is for client-side applications.

Regarding hasOwnProperty, I believe it's because it's trying to ignore inherited properties from the object's prototype chain. Maybe there are some packages that have really complicated header objects?

@ScriptSmith
Copy link
Copy Markdown
Contributor Author

ScriptSmith commented Sep 3, 2019

So how should form-data be interacting with the headers it's provided with? And how picky should it be?

I think it should just look at the object's keys, and not up the prototype chain. Therefore it makes sense to keep the hasOwnProperty check as it is.

I also think it shouldn't be picky about the types of header values, even though internally it only makes sense for them to be strings / numbers, because I think other users are relying on the non-standard getHeaders function to generate the content-type header:

form-data/lib/form_data.js

Lines 295 to 297 in 5c6bf52

var formHeaders = {
'content-type': 'multipart/form-data; boundary=' + this.getBoundary()
};

Therefore I think it should just use the simple definition that's already included, rather than any of the header definitions in http

What do you think @mapleeit?

mapleeit
mapleeit reviewed Sep 3, 2019
View reviewed changes
Comment thread index.d.ts Outdated
@mapleeit
Copy link
Copy Markdown
Collaborator

mapleeit commented Sep 3, 2019 •
edited
Loading

@ScriptSmith Agree with that. FormData.Headers could do. Although people may use http. OutgoingHttpHeaders param in most instances.

@mapleeit mapleeit merged commit 06568ed into form-data:master Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mapleeit mapleeit mapleeit left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ScriptSmith @coveralls @mapleeit @alexindigo