Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Socket's setEncoding(null) removes Stream encoder. #3643

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants

Rush commented Jul 5, 2012

Also according to Isaac's tips I have added a test for the new behavior. test/simple/test-net-socket-set-encoding-revert.js

@bnoordhuis bnoordhuis commented on an outdated diff Jul 5, 2012

@@ -430,6 +430,10 @@ function onread(buffer, offset, length) {
Socket.prototype.setEncoding = function(encoding) {
var StringDecoder = require('string_decoder').StringDecoder; // lazy load
+ if(!encoding) { // revert encoding if none provided
+ this._decoder = null;
+ return;
+ }
this._decoder = new StringDecoder(encoding);
@bnoordhuis

bnoordhuis Jul 5, 2012

Member

Why not a one-liner?

this._decoder = encoding ? new StringDecoder(encoding) : null;

Rush commented Jul 5, 2012

Good idea, fixed.

@bnoordhuis bnoordhuis commented on an outdated diff Jul 6, 2012

@@ -430,7 +430,7 @@ function onread(buffer, offset, length) {
Socket.prototype.setEncoding = function(encoding) {
var StringDecoder = require('string_decoder').StringDecoder; // lazy load
- this._decoder = new StringDecoder(encoding);
+ this._decoder = encoding ? new StringDecoder(encoding) : null; // revert encoding if none provided
@bnoordhuis

bnoordhuis Jul 6, 2012

Member

I hate to be that guy but lines should wrap at 80 characters. :-)

Rush commented Jul 6, 2012

I appreciate it Ben. ;) Fixed 80 columns wrapping also for the test code.

Member

bnoordhuis commented Jul 6, 2012

LGTM. An even better test would do two writes and check that the first is a string and the second one a buffer but the current test is good enough for me.

@isaacs You discussed this on IRC? Maybe you should take a look as well.

koichik commented Jul 7, 2012

@rushpl - Can you also fix http.IncomingMessage, fs.ReadStream, and stream's API doc?

Can one of the admins verify this patch?

Owner

jasnell commented Jun 1, 2015

@Rush ... I know it's been a while, but is this still something you want to pursue? If yes, the PR would need to be updated significantly. If not, we should likely go ahead and close.

Owner

jasnell commented Jun 24, 2015

Closing this, but tagging it to keep track of. The PR is out of date. @Rush ... if this is something that you'd still like to pursue, please open a new, updated PR.

/cc @isaacs @bnoordhuis

@jasnell jasnell closed this Jun 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment