Skip to content

Buffer.isEncoding regards an empty string as a valid encoding #9654

Closed
Closed
Buffer.isEncoding regards an empty string as a valid encoding#9654
Labels
bufferIssues and PRs related to the buffer subsystem.docIssues and PRs related to Node.js documentation.questionIssues asking questions about Node.js.

Description

@shinnn
shinnn
opened on Nov 17, 2016
  • Version: Node.js v7.1.0
  • Platform: Darwin Kernel Version 16.1.0

As stated in the issue title, Buffer.isEncoding('') returns true.

Actually, passing an empty string to encoding options of fs.readFile, Buffer.from or other functions supporting encoding doesn't result in an error, because they use the default encoding utf8 as a fallback in that case.

From this point of view, '' is also a valid encoding and Buffer.isEncoding('') === true looks fine. However, if we approve this behavior of Buffer.isEncoding, I think it's inconsistent that Buffer.isEncoding returns false when it takes other falsy values − false, null, undefined etc.

So, which design of Buffer.isEncoding is most reasonable?

  1. There is no problem with the current behavior of Buffer.isEncoding.
  2. Buffer.isEncoding should return false when it takes ''.
  3. Buffer.isEncoding should return true when it takes any falsy values.

I support the second one and can create a pull request.

I'd like to hear your opinion. Thanks.

Activity

  1. mscdex
    added
    bufferIssues and PRs related to the buffer subsystem.
    questionIssues asking questions about Node.js.
    on Nov 17, 2016

  2. BrandonZacharie commented on Nov 18, 2016

    @BrandonZacharie
    BrandonZacharie
    on Nov 18, 2016

    I also vote for option 2 ('' !== 'utf8').

  3. targos
    added
    docIssues and PRs related to Node.js documentation.
    on Jan 8, 2017

  4. Trott commented on Jul 15, 2017

    @Trott
    Trott
    on Jul 15, 2017
    Member

    @nodejs/buffer

  5. Trott
    added
    ctc-review
    on Aug 10, 2017

  6. Trott commented on Aug 10, 2017

    @Trott
    Trott
    on Aug 10, 2017
    Member

    I'm going to put a ctc-review label on this to at least try to get some input on whether this should be changed or not. @nodejs/ctc

  7. bnoordhuis commented on Aug 10, 2017

    @bnoordhuis
    bnoordhuis
    on Aug 10, 2017
    Member

    IMO, it would be consistent for Buffer.isEncoding() to accept any encoding that Buffer.from() accepts. That is:

    Buffer.from('x', '')     // ok, defaults to utf-8
Buffer.from('x', true)   // ok, defaults to utf-8
Buffer.from('x', false)  // ok, defaults to utf-8
// etc.

    The alternative is to make Buffer.from() and friends reject such values but that is almost certainly going to result in (possibly significant) ecosystem breakage.

    Another alternative is to simply state in the documentation that Buffer.from() accepts them from legacy reasons but that Buffer.isEncoding() is the one source of truth. Not a bad option either.

  8. tniessen commented on Aug 10, 2017

    @tniessen
    tniessen
    on Aug 10, 2017
    Member

    Another alternative is to simply state in the documentation that Buffer.from() accepts them from legacy reasons but that Buffer.isEncoding() is the one source of truth. Not a bad option either.

    I'd definitely support this, Buffer.isEncoding() should only return true for strings that specify an existing encoding, while Buffer.from may accept other inputs for backward compatibility.

  9. cjihrig commented on Aug 10, 2017

    @cjihrig
    cjihrig
    on Aug 10, 2017
    Contributor

    Another alternative is to simply state in the documentation that Buffer.from() accepts them from legacy reasons but that Buffer.isEncoding() is the one source of truth. Not a bad option either.

    I'm in favor of that approach.

  10. rvagg commented on Aug 14, 2017

    @rvagg
    rvagg
    on Aug 14, 2017
    Member

    I'm going to abstain from taking a position on this because the implications could be quite broad, but I will point out that this logic isn't quite right:

    However, if we approve this behavior of Buffer.isEncoding, I think it's inconsistent that Buffer.isEncoding returns false when it takes other falsy values − false, null, undefined etc.

    Just because '' is falsy doesn't mean we should include other falsy values in reasoning about acceptable categories. The broadest category of acceptable values is Strings, so we should just be restricting our arguments to what acceptable string contents are and just because a particular string is also in the category of "falsy" doesn't mean we should consider those values—similarly if we had an encoding that was also a number doesn't mean we should include numbers in our thinking about acceptable categories. A non-string value is clearly not an encoding type, so I believe we should be limiting the discussion to what string values are acceptable and whether '' can be treated as "default encoding", which seems reasonable, on the surface anyway.

  11. Trott
    added
    tsc-review
    and removed
    ctc-review
    on Sep 2, 2017

  12. BridgeAR commented on Apr 22, 2018

    @BridgeAR
    BridgeAR
    on Apr 22, 2018
    Member

    This got fixed in 452eed9

  13. BridgeAR
    closed this as completedon Apr 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bufferIssues and PRs related to the buffer subsystem.docIssues and PRs related to Node.js documentation.questionIssues asking questions about Node.js.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions