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 buffer method to Response. Resolves #51 #70

Closed
wants to merge 1 commit into from

Conversation

axfree
Copy link

@axfree axfree commented Jan 19, 2016

This patch add support for accessing the response body stream as a Buffer.
This is similar to arrayBuffer, but in nodejs style.

Use cases:

fetch('http://.../Some.TV.Show.S01E01.FeTCH.smi')
    .then(function(res) {
        return res.buffer();
    }).then(function(buffer) {
        var smi = convert(buffer, 'utf-8', 'CP949').toString();
        ...
    });


fetch('http://.../Some.TV.Show.S01E01.FeTCH.sub')
    .then(function(res) {
        return res.buffer();
    }).then(function(buffer) {
        var charset;
        var subType;

        if (buffer[0] == 0xef && buffer[1] == 0xbb && buffer[2] == 0xbf) {
            charset = 'utf8';
            buffer = buffer.slice(3);
        }
        if (buffer[0] == '1')
            subType = 'srt';
        else if (buffer[0] == '<' && buffer[1] == 'S')
            subType = 'smi';
        else
            ...

        var sub = convert(buffer, 'utf-8', charset).toString();
        ...
    });

@axfree axfree changed the title Add buffer method to Response. Add buffer method to Response. Resolves #51 Jan 21, 2016
@axfree axfree closed this Jan 21, 2016
@axfree axfree reopened this Jan 21, 2016
@bitinn
Copy link
Collaborator

bitinn commented Feb 1, 2016

Sorry for the delay in reply, but would just getting the body as a stream and converting it into Buffer better left for user? See #51

@axfree
Copy link
Author

axfree commented Feb 5, 2016

Because converting stream to buffer is not that trivial.
This is by far the simplest code I came up with

var toArray = require(‘stream-to-array’);

fetch('https://github.com/')
    .then(function(res) {
      return toArray(res.body);
    }).then(function (parts) {
      var buffers = []
      for (var i = 0, l = parts.length; i < l ; ++i) {
        var part = parts[i]
        buffers.push((part instanceof Buffer) ? part : new Buffer(part))
      }
      var buffer = Buffer.concat(buffers)
      console.dir(buffer);
      ...
    });

fetch does have similar code internally (_decode). Why not expose it for convenience.
This looks much better :)

fetch('https://github.com/')
    .then(function(res) {
      return res.buffer();
    }).then(function (buffer) {
      console.dir(buffer);
      ...
    });

This patch seems to fix #50 as well.

@jimmywarting
Copy link
Collaborator

Features

  • Stay consistent with window.fetch API.
  • ...

but would just getting the body as a stream and converting it into Buffer better left for user?

Fail to see your point there @bitinn. If it's going to be consistent with the fetch API then we should have a body.buffer() and/or body.arraybuffer() even if that means a degration of not using pipes.

I can also see some use cases for having that for some small response data where you don't want pipe it to a file/transformer or anything like that.
Checking for magic number (filetype) and for usage with protobuf.js (aka protocol buffer) just to mention a few

@bitinn
Copy link
Collaborator

bitinn commented Apr 2, 2016

@jimmywarting because I don't really see a point for arrayBuffer on server-side (as discussed in #51). I think when people need to buffer the full body (for non-text/html/json response), they are often missing the better solution to their problem.

For your specific use case, ie. file type detection, you probably only need the first a few chunks, I believe buffer is an overkill (it's a great time for a micro module, first-chunk and first-n-chunk are still available on npm!)

PS: node-fetch exists because people have argued for months about porting github's fetch polyfill to node.js: JakeChampion/fetch#9

@jimmywarting
Copy link
Collaborator

Please add this

@bitinn
Copy link
Collaborator

bitinn commented Aug 3, 2016

It's a bit late but good news nonetheless: buffer() has landed in v1.6.0

@bitinn bitinn closed this Aug 3, 2016
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