This function doesn't have to handle defaults -- nor should it. The two places that use this function both ensure encoding !== undefined. I also somewhat prefer the signature function getEncodingFromLabel(label) to align better with the spec.

ASCII whitespace (and ASCII whitespace only, not Unicode) needs to be trimmed as well per step 1 of the spec.

Just return enc w/o if should be fine and simpler.

How about just !!U_SUCCESS(status)? The == 1 looks kinda fragile.

Object.defineProperties is pretty slow to be used on per object construction. I don't see any harm in making kHandle configurable, enumerable, or writable, as it's a symbol property already (and it's what we do for URL). encoding, fatal, and ignoreBOM should all be defined on the prototype as getter functions per spec.

Use the non-deprecated Uint32Value(Local<Context>) signature of the function.

Independent of this PR, we should find a way to cache this ObjectTemplate.

Potentially dumb question: why not use ObjectWrap?

Ping this question...

For no particular reason other than ObjectWrap really isn't used anywhere else in core. I've actually been contemplating whether or not it is something we could eliminate at some point.

SetLength, too, takes number of entries as argument, not number of bytes.

Does the comment still apply?

Nope. Was planning to remove the whole section.

Is it documented anywhere how to get full ICU?

https://github.com/nodejs/node/blob/master/BUILDING.md#intl-ecma-402-support
+
https://github.com/nodejs/node/wiki/Intl (there is a link to it in the first fragment).

This should be better documented.

Per spec, the returned Uint8Array must be unpooled and have an entire block of ArrayBuffer dedicated to it.

[WHATWG Encoding Standard][] (+ second pair of brackets) for consistency?

while( -> while ( ?

stream:true -> stream: true ?

Note about defaults (false = strip the BOM)?

(false = strip the BOM)

That's correct.

var dec = new TextDecoder('utf-16be', { ignoreBOM: true });
[...dec.decode(new Uint8Array([0xfe, 0xff, 0x00, 0x20]))].map(str => str.codePointAt(0).toString(16));
  // ["feff", "20"]

var dec = new TextDecoder('utf-16be', { ignoreBOM: false });
[...dec.decode(new Uint8Array([0xfe, 0xff, 0x00, 0x20]))].map(str => str.codePointAt(0).toString(16));
  // ["20"]

I mean maybe it is worth to add a note about the default value.

Why we need to require Buffer if we state in the doc:

The Buffer class is a global within Node.js, making it unlikely that one would need to ever use require('buffer').Buffer

It's in a lint rule we use.

@TimothyGu Could you elaborate?

https://github.com/nodejs/node/blob/master/tools/eslint-rules/require-buffer.js

(added in #1794)

Why do we use it only in the lib/.eslintrc.yaml and not in the test/.eslintrc.yaml?

I'm guessing here: tests are supposed to be self-contained scripts, while everything in lib is user-visible and -contaminatable, like a seemingly innocent var Buffer = {} line in REPL. See nodejs/node-convergence-archive#21.

@TimothyGu Thank you.

If https://help.github.com/articles/organizing-information-with-tables/ works as advertised, github markdown tables would be easier to read in-source and to review.

As far as I understand (and I do not see anything in the referenced link) .. markdown tables do not support rowspan.

I wonder if withBOM (EDIT: or leaveBOM) would make more sense for this, if its not a fixed name due to the spec? I thought the docs might have had a typo and reversed the sense, because if the BOM is ignored it wouldn't be in the output, then I thought about it, and realized the sense of ignore is "don't remove from input stream if present". I'm not sure what would happen if the BOM is not in the input stream, though, would it get added to the decoded result?

ignoreBOM is fixed within the spec. To change this would require a change in the spec.

Understood. I know this is all experimental, but FYI, its not clear to me from current docs whether a BOM is added if not present, or just passed through ("ignored") if present.

The Web IDL spec treats null the same way as undefined. See https://heycam.github.io/webidl/#es-dictionary, step 4.1.2.

IDL recognizes all truthy values as true, so just options.stream ?.

This should coerce encoding to string rather than check its type, per IDL rules. See here for how it's done in URL.

Here too: null should be treated just like undefined. Also, options cannot be undefined here due to the default parameter above.

SharedArrayBuffer is not supported at this moment in Web platform APIs that do not have [AllowShared] extended attribute. See https://heycam.github.io/webidl/#es-buffer-source-types.

Yep, understood. Unfortunately, the separate process.binding('util').isArrayBuffer() and process.binding('util').isSharedArrayBuffer() method no longer exist. It looks like those were condensed recently into a single IsAnyArrayBuffer(). I think this is one we can safely ignore for now.

Seems bad to ignore, as becoming spec-complaint later will be a backward-incompatible breaking change.

Yeah, agreed. I've added the isArrayBuffer() util method back in to process.binding('util')

s/ArrayBuffer/Buffer/

This should use the Local<Context> version as well.

Put && !converter->ignoreBOM_ here as well?

This flag should only be set when there actually is a BOM, i.e. bomOffset != 0.

Not quite ...

In: https://encoding.spec.whatwg.org/#interface-textdecoder

* If encoding is UTF-8, UTF-16BE, or UTF-16LE, and ignore BOM flag and BOM seen flag are unset, then:
  * If token is U+FEFF, then set BOM seen flag.
  * Otherwise, if token is not end-of-stream, then set BOM seen flag and append token to output.
  * Otherwise, return output.

The ICU docs say:

For most Unicode charsets it is also possible to ignore the indicated number of initial stream bytes and start converting after them. However, there are stateful Unicode charsets (UTF-7 and BOCU-1) for which this will not work. Therefore, it is best to ignore the first output UChar instead of the input signature bytes.

AFAICT source is incremented by the signature bytes, so we are doing what the ICU docs advise not to do. Are the negative consequences applicable to us?

I do not believe so. TextDecoder does not support UTF-7 nor BOCU-1. We are only skipping the signature bytes for UTF-8, UTF-16le, and UTF-16be, none of which have this issue.

Ah, in that case LGTM.

input is an optional parameter too. See https://encoding.spec.whatwg.org/#interface-textencoder

You have to use template literal syntax to prevent symbols from getting converted to strings. See

Can this be omitted?

Also DataView. The %TypedArray% notation is generally reserved to specs, so do you think it is appropriate to use it here?

Object

uint8array?

Template string literal is needed here as well (to disallow symbols).

This check should technically be used for the user-visible getters and the methods as well.

Yeah, for now I'd rather leave it just here tho as it's not a check we commonly do.

FYI, we are doing this for URLs (see Web IDL create a operation function step 2.1.2.4). I'll defer to your opinion regarding these classes though.

I would recommend adding appropriate type-checking here if possible; not a big deal, but seems unnecessary to diverge from the spec and from URL in that way.

@domenic ... just want to make sure I'm clear on what kind of checking you're recommending? instanceof or is the more efficient hidden-symbol check ok?

Hidden symbol check for sure. Although ideally one that distinguishes encoder vs. decoder.

Ping this.

Same here. encoding and encode should have this check too.

For the time being, I'd prefer not.

:/ In this case I'd rather add a dummy property through a constructor, just so that subclasses that extend TextEncoder but override Symbol.toStringTag can work.

Ping this question...

Would a regex like /[\t\n\f\r ]/ be faster?

Not in my experience.

Rather than mentioning these as "unsupported", we can in fact say they don't necessarily need to be supported, since TextDecoder errors out on 'replacement'.

If we make sure no entries in encodings Map actually map to 'replacement' as value, we don't have to explicitly check for it here.

FYI, we are doing this for URLs (see Web IDL create a operation function step 2.1.2.4). I'll defer to your opinion regarding these classes though.

This unfortunately will not distinguish TextEncoder and TextDecoder objects, as both have this property defined.

FYI, Coverity is none too happy about this being re-enabled.