Skip to content
This repository was archived by the owner on Mar 10, 2020. It is now read-only.

[discuss] Switch to superagent instead of raw http.request #92

Merged
merged 1 commit into from
Nov 2, 2015

Conversation

dignifiedquire
Copy link
Contributor

So I have gotten superagent to work finally. But I sadly can not suggest to merge this. The reason being that the superagent browser part is much worse than the node part.
My general feeling is still that we should not use http.request directly but rather do some research into what request modules are out there and do a good job of what we need them to do in the browser as well as in node.

@victorb
Copy link
Contributor

victorb commented Oct 29, 2015

As I've said before, we should really look into fetch that exists natively
I'm browsers, have good shims for older browsers and node as well.
On Oct 29, 2015 14:54, "Friedel Ziegelmayer" [email protected]
wrote:

So I have gotten superagent to work finally. But I sadly can not suggest
to merge this. The reason being that the superagent browser part is much
worse than the node part.
My general feeling is still that we should not use http.request directly
but rather do some research into what request modules are out there and do

a good job of what we need them to do in the browser as well as in node.

You can view, comment on, or merge this pull request online at:

#92
Commit Summary

  • Switch to superagent instead of raw http.request

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#92.

@dignifiedquire
Copy link
Contributor Author

fetch does look interesting, this looks like a good polyfill https://github.com/matthew-andrews/isomorphic-fetch and also browser support looks quite decent http://caniuse.com/#search=fetch.

The other alternative that I know of that is widely used is https://github.com/request/request.

@daviddias
Copy link
Contributor

IIRC, this discussion started with the goal of fixing #69. From that, independently of the module or ES6/ES7 feature we use, do we have a new working solution, that doesn't loose any of the properties we have so far, such as recursive folder uploading, and that fixes that error?

If yes, let's move to that one. If not we should look into one. Our rule of thumb should be fixing bugs without breaking user space, and since the request-api.js component is abstracted by js-ipfs-api, we can change modules internally as we please and want to experiment things, that is super ok and welcome :)

@dignifiedquire
Copy link
Contributor Author

that doesn't loose any of the properties we have so far, such as recursive folder uploading, and that fixes that error?

Yes we have, this PR, but it only works with node.js the browser part is not working properly that's why I opened it with a discussion context.

@dignifiedquire
Copy link
Contributor Author

@jbenet @mappum I need some clarification on the capabilities of what you hacked together with vinyl-fs. As far as I can understand it supports recursive file uploads from nodejs but not the browser, as in the browser there is no filesystem from which we could upload, so there you can upload file objects or buffers, or strings, with the proper metadata, is that correct?

@jbenet
Copy link
Contributor

jbenet commented Nov 1, 2015

the browser can have a stream of (vinyl or other) files just the same,
representing deep hierarchies.

@dignifiedquire
Copy link
Contributor Author

@diasdavid please need some CR here, I think this is ready to be merged now, it resolves the issues from #69 which was the main goal but keeps full vinyl support. All tests are passing in node and the browser (as far as they were passing before). It's using request as it's the only one of the discussed modules that has proper streaming support so we can continue using get-files-stream.

@@ -31,7 +30,7 @@ function getFilesStream (files, opts) {
adder.add(vinylfs.src(file, srcOpts))

// if recursive, glob the contents
if (opts.r || opts.recursive) {
if (opts.recursive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why no more r?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm merging the r option with the recursive option in the configuration setup, so later down the line it's only one option to check.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, nice :)

@daviddias
Copy link
Contributor

it resolves the issues from #69 which was the main goal but keeps full vinyl support. All tests are passing in node and the browser (as far as they were passing before).

Sweet! Would you mind rebasing master onto this one? We just updated it to work with 0.3.9

@dignifiedquire
Copy link
Contributor Author

@diasdavid done

daviddias added a commit that referenced this pull request Nov 2, 2015
[discuss] Switch to superagent instead of raw http.request
@daviddias daviddias merged commit 77a7c14 into ipfs-inactive:master Nov 2, 2015
@daviddias
Copy link
Contributor

woot ! :D

@dignifiedquire dignifiedquire deleted the superagent branch November 2, 2015 08:50
@daviddias
Copy link
Contributor

@dignifiedquire seems that now 'cat' always returns a 'string', even in Node.js, where it should be a stream

@jbenet
Copy link
Contributor

jbenet commented Nov 2, 2015

it should be a stream in the browser too.

@dignifiedquire
Copy link
Contributor Author

@jbenet it's not possible to provide a real stream, as all browser requests are based on XHR, which has no concept of real streaming. So we can wrap the finished response in a stream, but not actually stream the data.

@daviddias
Copy link
Contributor

I believe what @jbenet meant is that the interface should be the same in both lands and therefore, if it returns a stream in one, should return a stream in the other.

@dignifiedquire
Copy link
Contributor Author

It makes sense to have that as a goal, but this would be a breaking api change in the browser.

@jbenet
Copy link
Contributor

jbenet commented Nov 2, 2015

I believe what @jbenet meant is that the interface should be the same in both lands and therefore, if it returns a stream in one, should return a stream in the other.

yes i meant this o/.

also, seriously? XHR cannot stream huge requests? does it have to support range queries then? (i believe we may already support these because seeking in videos works fine...

@daviddias
Copy link
Contributor

We were checking if the response was a stream before https://github.com/ipfs/js-ipfs-api/blob/cc9ec5138ab57225ba610434422c95bc65a26bc6/src/request-api.js#L56 and now we are glueing the chunks altogether and returning the thing.

also, seriously? XHR cannot stream huge requests? does it have to support range queries then? (i believe we may already support these because seeking in videos works fine...

Yeah, AFAIK, you always have to do some chunking, range or pagination magic. There is however, a stream-browserify module to offer Node.js streams in the browser, so that might have been what you had seen before (since we always browserified the code)

@dignifiedquire
Copy link
Contributor Author

XHR cannot stream huge requests? does it have to support range queries then? (i believe we may already support these because seeking in videos works fine...

There are issues at least around streaming, I think XHR2 allows for streaming the response though. Will need to do better research though.

now we are glueing the chunks altogether and returning the thing.

Yes, that was an oversight on my side, but should be easily fixable, by piping the response into a stream and returning that.

@daviddias
Copy link
Contributor

@dignifiedquire I'm trying to understand at the moment where in requestcan I check the header first and decide if I want to use the stream or have access to the body by itself

@dignifiedquire
Copy link
Contributor Author

@diasdavid this should explain it

  var request = require('request')
  request(
    { method: 'GET'
    , uri: 'http://www.google.com'
    , gzip: true
    }
  , function (error, response, body) {
      // body is the decompressed response body
      console.log('server encoded the data as: ' + (response.headers['content-encoding'] || 'identity'))
      console.log('the decoded data is: ' + body)
    }
  ).on('data', function(data) {
    // decompressed data as it is received
    console.log('decoded chunk: ' + data)
  })
  .on('response', function(response) {
    // unmodified http.IncomingMessage object
    response.on('data', function(data) {
      // compressed data as it is received
      console.log('received ' + data.length + ' bytes of compressed data')
    })
  })

@daviddias
Copy link
Contributor

The stream is in here https://github.com/ipfs/js-ipfs-api/blob/master/src/request-api.js#L67, but I only learn at https://github.com/ipfs/js-ipfs-api/blob/master/src/request-api.js#L51 if I should return as a stream or if I should parse the chunks. And it is not even returning, it is calling the cb with cb(null, stream)

Trying to think of a clean solution of how to patch this.

@dignifiedquire
Copy link
Contributor Author

@diasdavid let me do the patch, I think I know how to do it

@daviddias
Copy link
Contributor

awesome, I need to be absent for the next 1:30h, so feel free to push it :) #99

@daviddias
Copy link
Contributor

fixed on #99 :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants