Add multipart chunked flag

[simov]

Fixes #1243

That's just to show up the idea from my last comment. Basically I'm bringing back the code from v2.46 and I'm introducing a flag so that both implementations can exist together.

I'm proposing this syntax for non chunked/streamed multipart/related bodies

multipart: {
  data: []
}
// in place of the default one
multipart: []

Obviously that's ugly, but I can't think of anything more reasonable at this point. Once we agree on the syntax I'll add tests for it.

[simov]

This fix uploads successfully to both Couchdb and Gdrive, the first one uses fs.readFileSync and the second one uses fs.createReadStream for the body.

[nylen]

Yeah, that is quite ugly. If I'm reading #1243 correctly, this is only an issue with servers that interpret chunked requests incorrectly? Do we know of any such servers other than CouchDB, and how hard would it be for an external caller to implement this logic themselves?

If we go this route, I'd probably rather see separate options - multipart being the current implementation and something like multipartNonChunked being the old behavior.

[simov]

I don't know of any other server having problems with it, and most of them are implementing multipart/form-data anyway.

As for implementing outside of request - all the magic happens inside the multipart method, but I'm not sure if that's an option.

Can we add an additional chunked:true options to request. I have no idea if that could be used in any other context, so it might just add more noise to the API. I'm thinking of something more generic than multipartNonChunked

[nylen]

An additional flag option sounds fine to me. Let's call it multipartChunked if we do that though.

Or, we could do this:

// default, using chunked encoding (or not?)
{ multipart : [ ... ] }

// explicitly specify whether to use chunked encoding
{ multipart : {
    chunked : true,
    data : [ ... ]
} }
{ multipart : {
    chunked : false,
    data : [ ... ]
} }

[simov]

Yep, that was my initial intent by introducing an object for the multipart key - just to allow additional configuration for it. I'm generally for the object approach as it's more self contained + if we actually do have a separate multipart module in future, probably we'll need to pass options to it somehow.

Also I'm definitely for keeping the multipart:[] syntax for the current implementation, as it's more future proof and it contains the previous implementation in it + streaming of bodies.

Setting multipartChunked:false and keeping the multipart:[] array for both of the implementations doesn't sound that bad either. My initial try on this approach was to use some common header and then based on it to set the flag inside the multipart method. But then again it looked too hacky to me.

Let's see what @dscape thinks about it too.

[nylen]

Sounds good. I like having one option (that can be an object or an array), but having multipart : { data : [ ... ] } act differently than multipart : [ ... ] is too "magic" so I added the chunked property in there. Having an external library handle this in the future is another good reason to do it like that.

[simov]

Agreed, let's see if anyone else have something to add, otherwise I'm going with the multipart:[] and multipart:{chunked:false, data:[]} route.

[simov]

Currently the multipart.chunked check is a bit relaxed, meaning it can be omitted altogether (of course that should not be documented). Then I'm testing for all possible option permutations that I can think of.

[nylen]

This looks good to me, just needs docs.

[simov]

Done, the changes in the code example are not that much, I just couldn't figure out the punctuation, also the above example is using the commas at the end of the line as well.

[nylen]

Nice work as usual. Let's tweak the docs a little, do you mind pulling in my simov-multipart-chunked branch here, or giving feedback if you see any issues?

[simov]

It's definitely better, one small issue though. The default is not chunked:true instead there is no default at all. If you omit the chunked key it will be false.

What was the idea, is it supposed to be true by default?

[nylen]

You're right, I misread this line:

var chunked = (multipart instanceof Array) || multipart.chunked

I was thinking it would make sense to have multipart : [ ... ] behave the same as multipart : { data : [ ... ] } (in both of those cases, chunked is not specified so it would default to the current behavior which is true).

[simov]

Done. Also added a test case for it, and fixed a bug in the tests.

[nylen]

Thanks @simov!