Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

streams: inconsistency in setEncoding() #2533

Closed
koichik opened this Issue · 3 comments

2 participants

@koichik
Owner

In the API docs, streams module describes setEncoding(encoding) as follows:

Makes the data event emit a string instead of a Buffer. encoding can be 'utf8', 'ascii', or 'base64'.

But net module makes null a default, but does not describe its meaning:

Sets the encoding (either 'ascii', 'utf8', or 'base64') for data that is received. Defaults to null.

It is described in http module:

Set the encoding for the request body. Either 'utf8' or 'binary'. Defaults to null, which means that the 'data' event will emit a Buffer object.

However, current implementation does not do so. Actually, null means 'utf8' in all Readable Streams in core.

examle:

var http = require('http');
var server = http.createServer(function(req, res) {
  res.end('あいうえお', 'utf8');
}).listen(3000, function() {
  http.get({port: 3000}, function(res) {
    res.setEncoding(null);
    res.on('data', function(data) {
      console.log(typeof data);
      console.log(data);
    });
    res.on('end', function() {
      server.close();
    });
  });
});

result:

$ ./node /tmp/setEncoding.js
string
あいうえお

Therefore, we should fix the API docs, implementations, or both. I would like to fix implementations so that http module describes it. But in v0.6, probably it is only the API docs that we can fix.

Thoughts?

@bnoordhuis

Therefore, we should fix the API docs, implementations, or both. I would like to fix implementations so that http module describes it. But in v0.6, probably it is only the API docs that we can fix.

Yes.

Is stream.setEncoding() even the proper place for that kind of functionality? stream.Stream is essentially an an abstract base class, the best a .setEncoding() method could do is to give a hint to the concrete implementation - unless you want to add a (moderately?) complex conversion layer.

@koichik
Owner

Let's postpone this with #2524.

@koichik
Owner

Closed by #3209

@koichik koichik closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.