buffer: implement WHATWG Encoding Standard API

[jasnell]

Provide an (initially experimental) implementation of the WHATWG Encoding Standard API (TextDecoder and TextEncoder). The is the same API implemented on the browser side.

By default, with small-icu, only the UTF-8, UTF-16le and UTF-16be decoders are supported. With full-icu enabled, every encoding other than iso-8859-16 is supported.

This provides a basic test, but does not include the full web platform tests. Note: many of the web platform tests for this would fail by default because we ship with small-icu by default.

The implementation is added without changing any of the existing encoding support in core.

Refs: https://encoding.spec.whatwg.org/

/cc @domenic @TimothyGu @addaleax

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

buffer

[mscdex]

What's special about iso-8859-16?

[jasnell]

It's not implemented by ICU at all, even with full-icu

[TimothyGu]

Some preliminary comments. I'll need some time to digest the intricacies of BOM and the various flags related to it.

[TimothyGu]

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.

[TimothyGu]

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

[TimothyGu]

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

[TimothyGu]

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

[TimothyGu]

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.

[TimothyGu]

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

[TimothyGu]

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

Potentially dumb question: why not use ObjectWrap?

[TimothyGu]

Ping this question...

[jasnell]

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.

[TimothyGu]

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

[TimothyGu]

Does the comment still apply?

[jasnell]

Nope. Was planning to remove the whole section.

[TimothyGu]

Is it documented anywhere how to get full ICU?

[vsemozhetbyt]

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).

[jasnell]

This should be better documented.

[TimothyGu]

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

[vsemozhetbyt]

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

[vsemozhetbyt]

while( -> while ( ?

[vsemozhetbyt]

stream:true -> stream: true ?

[vsemozhetbyt]

Note about defaults (false = strip the BOM)?

[TimothyGu]

(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"]

[vsemozhetbyt]

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

[vsemozhetbyt]

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

[TimothyGu]

It's in a lint rule we use.

[vsemozhetbyt]

@TimothyGu Could you elaborate?

[TimothyGu]

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

(added in #1794)

[vsemozhetbyt]

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

[TimothyGu]

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.

[vsemozhetbyt]

@TimothyGu Thank you.

[jasnell]

Updated

[sam-github]

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.

[jasnell]

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

[sam-github]

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?

[jasnell]

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

[sam-github]

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.

[TimothyGu]

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

[TimothyGu]

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

[TimothyGu]

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

[TimothyGu]

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

[TimothyGu]

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.

[jasnell]

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.

[domenic]

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

[jasnell]

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

[TimothyGu]

s/ArrayBuffer/Buffer/

[TimothyGu]

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

[TimothyGu]

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

[TimothyGu]

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

[jasnell]

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.

[TimothyGu]

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?

[jasnell]

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.

[TimothyGu]

Ah, in that case LGTM.

[TimothyGu]

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

[TimothyGu]

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

node/lib/internal/url.js

Line 56 in 448c4c6

const str = `${val}`;

.

[TimothyGu]

Can this be omitted?

[jasnell]

Updated :-)
@TimothyGu ... as always, I'm loving the thorough review :-)

[TimothyGu]

Looking much better, thanks a lot!

[TimothyGu]

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

[TimothyGu]

Object

[TimothyGu]

uint8array?

[TimothyGu]

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

[TimothyGu]

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

[jasnell]

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

[TimothyGu]

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.

[domenic]

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.

[jasnell]

@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?

[domenic]

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

[TimothyGu]

Ping this.

[TimothyGu]

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

[jasnell]

For the time being, I'd prefer not.

[TimothyGu]

:/ 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.

[TimothyGu]

Ping this question...

[TimothyGu]

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

[jasnell]

Not in my experience.

[TimothyGu]

A bug I noticed: https://encoding.spec.whatwg.org/#dom-textdecoder-decode step 1 unsets the BOM seen flag if do not flush flag resulted from a previous run is not set (i.e. this is either the first call to decode() or the first call after a decode() call with { stream: false }). However, this doesn't appear to be so:

> var dec = new buffer.TextDecoder('utf-8');
> [...dec.decode(Uint8Array.of(0xEF, 0xBB, 0xBF, 0x20))].map(str => str.codePointAt(0).toString(16));
[ '20' ]
> [...dec.decode(Uint8Array.of(0xEF, 0xBB, 0xBF, 0x20))].map(str => str.codePointAt(0).toString(16));
[ 'feff', '20' ]

The correct behavior seems to also be the one implemented by Chrome:

> var dec = new TextDecoder('utf-8');
> [...dec.decode(Uint8Array.of(0xEF, 0xBB, 0xBF, 0x20))].map(str => str.codePointAt(0).toString(16));
[ '20' ]
> [...dec.decode(Uint8Array.of(0xEF, 0xBB, 0xBF, 0x20))].map(str => str.codePointAt(0).toString(16));
[ '20' ]

[TimothyGu]

One more observation...

[TimothyGu]

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'.

[TimothyGu]

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.

[jasnell]

@TimothyGu ... updated!

[TimothyGu]

LGTM as is, but some further observations are below. WPT integration can be worked on after this PR is merged.

I do have one more suggested edit: TimothyGu@0d0f7fc, which eliminates the need to copy from a Buffer to an Uint8Array.

[TimothyGu]

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.

[TimothyGu]

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

[bnoordhuis]

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

[gibfahn]

Release team decided not to land on v6.x, if you disagree let us know.