Skip to content

Conversation

@omsmith
Copy link
Contributor

@omsmith omsmith commented Apr 29, 2016

The default encoding was switched to utf8 in nodejs/node#5522

@omsmith
Copy link
Contributor Author

omsmith commented Apr 29, 2016

Fixes #5102

@omsmith omsmith mentioned this pull request Apr 29, 2016
@daleharvey
Copy link
Contributor

To be honest I dont 100% understand this issue, however its certainly related #2580, would be good to have this settled, is binary deprecated / should we switch to utf8, or is being explicit about the type we want all thats needed and this PR resolves everything?

@daleharvey
Copy link
Contributor

@omsmith So I screwed up and forgot to actually add the travis stable run to verify that this fixes the tests, any chance you could rebase this on master to verify the fix works in node 6? (added 9ec5cea)

The default encoding was switched to utf8 in nodejs/node#5522
@daleharvey
Copy link
Contributor

Awesome thanks

@nolanlawson
Copy link
Member

@daleharvey The difference is that 'binary' is no longer implicit in Node 6. I actually recall seeing in the documentation some time ago that that API was subject to change, so I probably should have added 'binary' earlier.

I'm +1 on this change because it matches the Node <6 behavior, as well as the behavior in the browser and in CouchDB (except for strings, where CouchDB gets weird because Erlang).

@daleharvey
Copy link
Contributor

Awesome, so this will also close out #2580 right? also 👍 when green

@omsmith
Copy link
Contributor Author

omsmith commented Apr 29, 2016

@daleharvey #2580 is closed out by nodejs/node#3441

@daleharvey
Copy link
Contributor

Awesome thanks

@daleharvey
Copy link
Contributor

Great all passing :)

@daleharvey daleharvey merged commit 3d7d497 into apache:master Apr 29, 2016
@omsmith omsmith deleted the fix-node6-hash branch March 12, 2019 02:04
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