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 multipart chunked flag #1253

Merged
merged 6 commits into from
Nov 11, 2014
Merged

Add multipart chunked flag #1253

merged 6 commits into from
Nov 11, 2014

Conversation

simov
Copy link
Member

@simov simov commented Nov 8, 2014

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
Copy link
Member Author

simov commented Nov 8, 2014

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
Copy link
Member

nylen commented Nov 9, 2014

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
Copy link
Member Author

simov commented Nov 9, 2014

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
Copy link
Member

nylen commented Nov 10, 2014

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
Copy link
Member Author

simov commented Nov 10, 2014

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
Copy link
Member

nylen commented Nov 10, 2014

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
Copy link
Member Author

simov commented Nov 10, 2014

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
Copy link
Member Author

simov commented Nov 10, 2014

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
Copy link
Member

nylen commented Nov 10, 2014

This looks good to me, just needs docs.

@simov
Copy link
Member Author

simov commented Nov 10, 2014

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
Copy link
Member

nylen commented Nov 10, 2014

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
Copy link
Member Author

simov commented Nov 10, 2014

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
Copy link
Member

nylen commented Nov 10, 2014

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
Copy link
Member Author

simov commented Nov 11, 2014

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

@nylen
Copy link
Member

nylen commented Nov 11, 2014

Thanks @simov!

nylen added a commit that referenced this pull request Nov 11, 2014
@nylen nylen merged commit 408a7bb into request:master Nov 11, 2014
@simov simov mentioned this pull request Nov 22, 2014
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.

Request timeout on multipart
2 participants