Skip to content

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

Merged
alexindigo merged 6 commits intomasterfrom
unknown repository
Dec 5, 2012
Merged

Allow custom headers and pre-known length in parts#17
alexindigo merged 6 commits intomasterfrom
unknown repository

Conversation

@benbuckman
Copy link
Copy Markdown

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)

@benbuckman
Copy link
Copy Markdown
Author

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

Comment thread lib/form_data.js
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 {
...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@alexindigo Would a PR help ?

@benbuckman
Copy link
Copy Markdown
Author

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.

@alexindigo
Copy link
Copy Markdown
Member

Hi Ben,

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

Thank you.

@ghost ghost assigned alexindigo Nov 4, 2012
@benbuckman
Copy link
Copy Markdown
Author

Will do.

@benbuckman
Copy link
Copy Markdown
Author

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

@alexindigo
Copy link
Copy Markdown
Member

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
Allow custom headers and pre-known length in parts
@alexindigo alexindigo merged commit f180a85 into form-data:master Dec 5, 2012
@alexindigo
Copy link
Copy Markdown
Member

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

@alexindigo
Copy link
Copy Markdown
Member

Published new version to npm.

@benbuckman
Copy link
Copy Markdown
Author

Thanks!

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.

3 participants