Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

A minor change to how field names are encoded #161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

A minor change to how field names are encoded #161

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 14 additions & 9 deletions lib/form_data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ function FormData() {
FormData.LINE_BREAK = '\r\n';
FormData.DEFAULT_CONTENT_TYPE = 'application/octet-stream';

FormData.prototype.append = function(field, value, options) {
FormData.prototype.append = function(field, value, options, encodeFieldNamesWithoutQuotes) {

options = options || {};

Expand All @@ -61,7 +61,7 @@ FormData.prototype.append = function(field, value, options) {
return;
}

var header = this._multiPartHeader(field, value, options);
var header = this._multiPartHeader(field, value, options, encodeFieldNamesWithoutQuotes);
var footer = this._multiPartFooter();

append(header);
Expand Down Expand Up @@ -119,7 +119,7 @@ FormData.prototype._trackLength = function(header, value, options) {
// inclusive, starts with 0
next(null, value.end + 1 - (value.start ? value.start : 0));

// not that fast snoopy
// not that fast snoopy
} else {
// still need to fetch file size from fs
fs.stat(value.path, function(err, stat) {
Expand All @@ -137,11 +137,11 @@ FormData.prototype._trackLength = function(header, value, options) {
});
}

// or http response
// or http response
} else if (value.hasOwnProperty('httpVersion')) {
next(null, +value.headers['content-length']);

// or request stream http://github.com/mikeal/request
// or request stream http://github.com/mikeal/request
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind to clean up unnecessary changes like this?
And add tests for the use case?

Thank you.

} else if (value.hasOwnProperty('httpModule')) {
// wait till response come back
value.on('response', function(response) {
Expand All @@ -150,7 +150,7 @@ FormData.prototype._trackLength = function(header, value, options) {
});
value.resume();

// something else
// something else
} else {
next('Unknown stream');
}
Expand All @@ -159,7 +159,7 @@ FormData.prototype._trackLength = function(header, value, options) {
};

// TODO: Use request's response mime-type
FormData.prototype._multiPartHeader = function(field, value, options) {
FormData.prototype._multiPartHeader = function(field, value, options,encodeFieldNamesWithoutQuotes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codacy Issue found: This function's cyclomatic complexity is too high. (9)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend to move this parameter to the options, since it may be passed here anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, thanks

// custom header specified (as string)?
// it becomes responsible for boundary
// (e.g. to handle extra CRLFs on .NET servers)
Expand All @@ -171,7 +171,12 @@ FormData.prototype._multiPartHeader = function(field, value, options) {
var contentType = this._getContentType(value, options);

var contents = '';
var headers = {
var headers = encodeFieldNamesWithoutQuotes?{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think more explicit syntax will be more readable. Thanks.

// add custom disposition as third element or keep it two elements if not
'Content-Disposition': ['form-data', 'name=' + field].concat(contentDisposition || []),
// if no content type. allow it to be empty array
'Content-Type': [].concat(contentType || [])
}:{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And probably makes sense to move it into separate function.

// add custom disposition as third element or keep it two elements if not
'Content-Disposition': ['form-data', 'name="' + field + '"'].concat(contentDisposition || []),
// if no content type. allow it to be empty array
Expand Down Expand Up @@ -365,7 +370,7 @@ FormData.prototype.submit = function(params, cb) {
host: params.hostname
}, defaults);

// use custom params
// use custom params
} else {

options = populate(params, defaults);
Expand Down