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.

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

Add support for multipart form data via the request options #925

Closed
wants to merge 8 commits into from

Conversation

FredKSchott
Copy link
Contributor

(See original proposal here #924)

Summary

Instead of creating your own form for multipart uploads and having to append data separately, pass the data you'd like to attach via the request options argument. For more advanced cases where the third append argument is required, the current solution can still be used.

Before

// From README#Forms
var options = { /* ... */ };
var r = request.post(options, someOptionalCallback);
var form = r.form()
form.append('my_field', 'my_value')
form.append('my_buffer', new Buffer([1, 2, 3]))
// ...

After

var options = {
    url: 'http://service.com/upload',
    formMPU: {
        my_filed: 'my_value',
        my_buffer: new Buffer([1,2,3])
    }
};
request(options, someOptionalCallback);

If the server on the endpoint does not support SSL correctly, a response.client object may not be available. Check before reading properties from response.client.
@FredKSchott
Copy link
Contributor Author

Forgot to mention in the PR, this includes tests for the new option.

@LoicMahieu
Copy link
Member

+1 For this feature! It would be awesome!
Just... I am skeptical about the "formMPU" key ... Maybe "formdata" or something more representative. (Just a matter of vocabulary)

@crocket
Copy link
Contributor

crocket commented Jul 9, 2014

I also prefer "formdata" to "formMPU".
This makes multipart/form-data requests compatible with promisification.

@@ -0,0 +1,77 @@
try {
Copy link
Member

Choose a reason for hiding this comment

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

This try should be removed and form-data should be added to devDependencies.

@FredKSchott
Copy link
Contributor Author

Re naming: In my original proposal I admitted I couldn't think of a good name :) I like formdata as well, I'll rename it.

@FredKSchott
Copy link
Contributor Author

Hmm, Travis wasn't running test-form.js or test-form-data.js before because of that try-catch. Now that it's gone, it's having trouble finding the mime module, which is a normal dependency.

It looks like Travis isn't calling npm install before running your tests, which is preventing a lot of test suites from running.

@FredKSchott
Copy link
Contributor Author

Also an open question: What should happen when form & formdata are present? Currently formdata takes precedence, but is this desired? (This should never really happen since both do conflicting things, but it is possible.)

@mikeal
Copy link
Member

mikeal commented Jul 9, 2014

@FredKSchott there are lots of options that conflict when used together, i wouldn't worry about it too much :)

@mikeal
Copy link
Member

mikeal commented Jul 9, 2014

you might wanna pull latest, the mime module was swapped for 'mime-types' and is updated in the test that is in HEAD.

@FredKSchott
Copy link
Contributor Author

Ah, yea that was it. good catch.

Alright, comments addressed, tests passing, and README updated. Should be ready for final review / merge. Let me know if you need anything else, and feel free to edit the README if you'd rather explain the new option yourself.

"optionalDependencies": {
"tough-cookie": ">=0.12.0",
"form-data": "~0.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

You know what, this should actually be a regular ol' dependency. I'm just not sold anymore on the optionalDependency thing, and making it a devDependency was not correct, that's my bad, I apologize :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I didn't even think about it either when I made the change

@mikeal
Copy link
Member

mikeal commented Jul 10, 2014

one last comment/change cause I was wrong about the devDep :)

@nylen
Copy link
Member

nylen commented Jul 10, 2014

@mikeal would you be open to porting the test suite to mocha or similar?

@FredKSchott
Copy link
Contributor Author

comment addressed

@mikeal
Copy link
Member

mikeal commented Aug 27, 2014

This no longer merges cleanly and I think it refers to an old version of form data. Sorry this got lost in the pile would, love a clean PR though :)

@mikeal mikeal closed this Aug 27, 2014
Checking for SSL fault on connection before reading SSL properties
@FredKSchott FredKSchott deleted the form-mpu branch August 28, 2014 02:19
@FredKSchott FredKSchott restored the form-mpu branch August 28, 2014 02:19
@FredKSchott FredKSchott reopened this Aug 28, 2014
@FredKSchott
Copy link
Contributor Author

yea there's some wierdness going on here, I cleaned it up on my local repo but every pull request keeps including these other commits. I can't seem to fix it so I'll just keep it closed.

@crocket's PR looks fine, feel free to merge his instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants