apache / nano

Nano is now part of Apache CouchDB. Repo moved to https://GitHub.com/apache/couchdb-nano
https://github.com/apache/couchdb-nano
Other
1.13k stars 157 forks source link

Streamable multipart attachments #300

Closed stevebest closed 7 years ago

stevebest commented 8 years ago

This is all it takes, really!

Tests are soon to { follow: true }.

jo commented 8 years ago

Awesome! The test can be similar to this one: https://github.com/dscape/nano/blob/master/tests/integration/multipart/insert.js#L57-L69

jo commented 8 years ago

@dscape All ios targets fail because of missing nvm. What can we do? I would suggest to just remove the os matrix.

stevebest commented 8 years ago

:hankey:, it seems like I trusted request too much. It uses outdated combined-stream module to build multipart/related requests.

Also, request switches to Transfer-Encoding: chunked as soon as it sees a stream in mulitpart array, whether a stream has a known 'length' or not. And I'm not sure if CouchDB even supports this encoding.

jo commented 8 years ago

Would it help if request would update its combined-stream dependency?

stevebest commented 8 years ago

Uuugh, it gets worse. CouchDB silently hangs on Transfer-Encoding: chunked requests, so the only option is to not use it -- at least until 1.7.

On the bright side, request allows specifying multipart option in the following form, which forces it to not use chunked encoding:

request({
  method: 'PUT',
  uri: 'http://localhost:5984/mydb/mydoc',
  multipart: {
     chunked: false,   //  HUZZAH!!!
     data: [
       { content_type: 'application/json', length: 42, body: '{ ..., "_attachments": { "image.jpg": { ... } } }' },
       { content_type: 'image/jpg', length: 43234, body: fs.createReadStream('image.jpg') },
       /* etc */
     ]
  }
}, callback);

nano should probably use this form. But it won't help.

Support for streams in request is broken anyways: when the encoding is not chunked, the stream gets wrapped in Buffer, which makes no sense at all.

So yeah, #299 would actually require changes in both nano and request, and it's not that we just have to update dependencies.

carlosduclos commented 7 years ago

Hi, The NanoJS repository has been merged into apache/couchdb-nano. Could you close this pull request and open it there instead? Thanks in advance

stevebest commented 7 years ago

Closing because #299 is obsolete.