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

Read automatically converts payload from Buffer to UTF-8 String #75

Closed
christophercliff opened this issue Mar 5, 2015 · 6 comments
Closed
Assignees
Milestone

Comments

@christophercliff
Copy link

@christophercliff christophercliff commented Mar 5, 2015

This causes problems when doing a GET on a binary file. Curious why not leave the payload as a Buffer?

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Mar 5, 2015

Looking at the code a bit, it's not doing that, unless the content-type is saying it can.
https://github.com/hapijs/wreck/blob/master/lib/index.js#L257
Unless you're forcing json (L247) or the response is application/json, it shouldn't do that.

@christophercliff

This comment has been minimized.

Copy link
Author

@christophercliff christophercliff commented Mar 5, 2015

This block:

wreck/lib/index.js

Lines 397 to 399 in 12011cd

if (payload instanceof Buffer) {
payload = payload.toString();
}

Looks like it was introduced here and here, although not clear why.

To clarify, the problem is that you can't encode a Buffer as UTF-8 then back to a Buffer w/o losing data.

@christophercliff

This comment has been minimized.

Copy link
Author

@christophercliff christophercliff commented Mar 6, 2015

Adding @cjihrig

@cjihrig

This comment has been minimized.

Copy link
Contributor

@cjihrig cjihrig commented Mar 6, 2015

I can't remember why I added the if (payload instanceof Buffer), but if you look at the diff from #6, payload.toString() was being called unconditionally before that. FWIW, all the tests still pass if that if statement is removed, although it would represent a breaking change.

@christophercliff

This comment has been minimized.

Copy link
Author

@christophercliff christophercliff commented Mar 6, 2015

From the README:

The payload in the form of a Buffer or (optionally) parsed JavaScript object (JSON).

So removing that statement would be a bugfix, no?

@grinnellian

This comment has been minimized.

Copy link
Contributor

@grinnellian grinnellian commented Jun 26, 2015

Also having binary file GETS fail:

var uri = "https://phs.googlecode.com/files/Download%20File%20Test.zip";

var options = {};
options.headers = {
  Accept: "application/x-zip-compressed"
};

Wreck.get(uri, options, function (err, response, payload) {
  if (err) throw err;
  if (!Buffer.isBuffer(payload)) throw new Error("Payload is not a buffer”);
  console.log(“So far so good”);
});
@geek geek self-assigned this Jul 1, 2015
grinnellian added a commit to grinnellian/wreck that referenced this issue Jul 1, 2015
geek added a commit that referenced this issue Jul 1, 2015
Payload returns as a buffer. Fixes Issue #75
@geek geek closed this Jul 1, 2015
@geek geek added this to the 6.0.0 milestone Jul 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.