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

buffer.toString() no longer accepts null as encoding #13936

Closed
ibc opened this issue Jun 26, 2017 · 10 comments
Closed

buffer.toString() no longer accepts null as encoding #13936

ibc opened this issue Jun 26, 2017 · 10 comments
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@ibc
Copy link

ibc commented Jun 26, 2017

  • Version: v8.1.2
  • Platform: Darwin ibc-macbook 16.6.0 Darwin Kernel Version 16.6.0: Fri Apr 14 16:21:16 PDT 2017; root:xnu-3789.60.24~6/RELEASE_X86_64 x86_64
  • Subsystem:

According to the doc, buffer.toString([encoding[, start[, end]]]):

encoding The character encoding to decode to. Default: 'utf8'

In Node 4/5/6/7 this works:

buffer.toString(null, 0);
buffer.toString(undefined, 0);

However, in Node 8, null does not work and produces TypeError: Unknown encoding: null.

Example:

var buf = Buffer.from("hello", 'utf8');

buf.toString(null);
// => TypeError: Unknown encoding: null

Not sure why null is no longer considered "use the default value".

@vsemozhetbyt vsemozhetbyt added buffer Issues and PRs related to the buffer subsystem. v8.x labels Jun 26, 2017
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 26, 2017

It is most likely caused by the #11120 and listed among the semver-major changes.

@mscdex
Copy link
Contributor

mscdex commented Jun 26, 2017

To be fair though, fixing this use case and the original use case (passing 0 as the first argument to toString()) detailed in the PR that made this change would be as simple as changing === undefined to == undefined.

Whether we'd want to support such a change is another question (e.g. would anyone complain about not accepting false for example?).

@ibc
Copy link
Author

ibc commented Jun 26, 2017

I understand the rationale given in #11120. However, shouldn't null be also valid (and not just undefined) for encoding?

@seishun
Copy link
Contributor

seishun commented Jun 27, 2017

null was never documented as a "valid" encoding, so I don't see any reason to support it. It's just more ground for confusion.

@benjamingr
Copy link
Member

Another +1 from me for keeping the 8.x behavior. I did not review the original PR because I saw it already had lots of love - but I'm definitely in favor of the behavior change.

@addaleax addaleax removed the v8.x label Jun 30, 2017
@addaleax
Copy link
Member

@vsemozhetbyt One small request … can you leave off the version-specific labels if the issue is the same on master? Otherwise it seems like the issue is only occuring on v8.x. :)

@seishun
Copy link
Contributor

seishun commented Jun 30, 2017

Closing as expected behavior.

@seishun seishun closed this as completed Jun 30, 2017
@ibc
Copy link
Author

ibc commented Jun 30, 2017

Why is this the expected behavior? The signature of the method is:

buffer.toString([encoding[, start[, end]]])

So encoding is an optional argument. However, where is it written that passing encoding = undefined means "use the default value"? Is there any rule out there stating that undefined is the only way to mean that while null is invalid?

@seishun
Copy link
Contributor

seishun commented Jun 30, 2017

That's how default arguments work in JavaScript in general:

> function foo(arg = 5) { console.log(arg); }
> foo()
5
> foo(undefined)
5
> foo(null)
null

However, it might be a good idea to document that explicitly.

So encoding is an optional argument.

Per the signature, it's only optional if start and end aren't specified, otherwise it would be buffer.toString([encoding][, start[, end]]) or something like that.

@ibc
Copy link
Author

ibc commented Jun 30, 2017

Good and clarifying code snip. Thanks.

Per the signature, it's only optional if start and end aren't specified, otherwise it would be buffer.toString([encoding][, start[, end]]) or something like that.

Completely right. Thanks, I was wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants