Allow custom headers and pre-known length in parts #17

Merged
merged 6 commits into from Dec 5, 2012

Conversation

Projects
None yet
3 participants
Contributor

benbuckman commented Oct 23, 2012

We're using this module (inside request) to stream files from 3rd-party file storage services straight through (as multipart POSTs) to our REST API. Our API expects special headers on each part, but the module currently doesn't support that. This change allows an options.header parameter to override the header string passed to each part (including the boundary and CRLFs - the latter being particularly important, similar to Request PR #272).

Thank you!

(More additions below)

Contributor

benbuckman commented Oct 23, 2012

A note on the header option: I left it as a string so the calling function can customize it 100%, including line breaks (or not), spaces between key:value or not, etc. But it might be prettier if it took an object and converted it; or if it checked for string/object and converted in the latter case. What's your preference @felixge ?

Thanks

+ // custom header specified (as string)?
+ // it becomes responsible for boundary
+ // (e.g. to handle extra CRLFs on .NET servers)
+ if (options.header != null) {
@ghost

ghost Oct 23, 2012

RE: header key/value option

Maybe something like this ...

``` javascript`
...
if (typeof(options.header) === 'object') {
for (key in options.header) {
header += key + ': ' options.header[key] + '\r\n'
}
} else if (typeof(options.header) === 'string') {
header = options.header
} else {
...

@Marsup

Marsup Jan 14, 2015

@alexindigo Was this comment answered somewhere ? I'm currently in the middle of this nightmare.

@alexindigo

alexindigo Jan 14, 2015

Owner

@Marsup too bad to hear FormData is failing you. Can you describe your situation? Maybe together we can figure out something.

@Marsup

Marsup Jan 14, 2015

@alexindigo I need to add custom headers to each part I append, this snippet of code would make this possible it seems.

@alexindigo

alexindigo Jan 14, 2015

Owner

This snippet only saves you one loop and if you came all the way to generate your own boundaries and pretty much to reimplement logic of the whole function, you can pass your custom headers as string with no greater complexity. Right? Or did I miss something?

@Marsup

Marsup Jan 14, 2015

I want to add a file, so let form-data do its work on boundary, filename, mime etc... But I also want to add a few headers. Maybe this snippet is not perfect but the 1st "if" would probably solve this if put after (currently line 162).

@alexindigo

alexindigo Jan 14, 2015

Owner

I mean, can you provide your code for better understanding, why this example doesn't work for you.

var FormData = require('form-data');
var form = new FormData();

var options = {
  header: FormData.LINE_BREAK + '--' + form.getBoundary() + FormData.LINE_BREAK + 'X-Custom-Header: 123' + FormData.LINE_BREAK + FormData.LINE_BREAK
};

form.append('my_thing', theThing, options);
@Marsup

Marsup Jan 14, 2015

I'd have to recreate the whole Content-Disposition, Content-Type, name, filename, for that I'd have to add mime as direct dependency and basically copy most of _multiPartHeader, this doesn't make sense.

Isn't this much clearer this way ?

var FormData = require('form-data');
var form = new FormData();

var options = {
  header: {
    'X-Custom-Header': 123
  }
};

form.append('my_thing', theThing, options);
@alexindigo

alexindigo Jan 14, 2015

Owner

Yes, you're right. This is legit issue. Let me see what I can do. Idea was to freeze new features while packing everything for 1.0 release. I'll get back to you soon.

Contributor

benbuckman commented Oct 24, 2012

The additional commit (e2ac039) is for the same use case: We already know the file size of an inbound transfer, and want to stream it directly outbound to a server that requires a Content-Length, with minimal latency. Currently we have to download the file to disk to get the size, or wait for the inbound stream to end to gets its content-length, which both introduce the latency again. This change allows options.knownLength to specify the size a priori.

(Also fixed a minor typo.)

Thank you.

Owner

alexindigo commented Nov 4, 2012

Hi Ben,

Can you add a test to show off the issue you're solving?

Thank you.

@ghost ghost assigned alexindigo Nov 4, 2012

Contributor

benbuckman commented Nov 5, 2012

Will do.

Contributor

benbuckman commented Nov 20, 2012

Following up on this, is there anything else I should add to this pull request for it to be accepted?
Thanks!

Owner

alexindigo commented Nov 20, 2012

Hey, sorry, got carried away. Let me take a look and I'd get back to you shortly.
Thank you for the PR.

alexindigo added a commit that referenced this pull request Dec 5, 2012

Merge pull request #17 from DocuSignDev/master
Allow custom headers and pre-known length in parts

@alexindigo alexindigo merged commit f180a85 into form-data:master Dec 5, 2012

Owner

alexindigo commented Dec 5, 2012

Thank you for the PR, and again terrible sorry for all the delays. :)

Owner

alexindigo commented Dec 5, 2012

Published new version to npm.

Contributor

benbuckman commented Dec 5, 2012

Thanks!

@benbuckman benbuckman referenced this pull request in request/request Mar 1, 2013

Closed

Use latest form-data 0.0.5 #385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment