Repository: nodejs/node
Pull request: lib: unify ICU and no-ICU TextDecoder

Better reviewed with whitespace ignored

1. Fix #61411 
2. Add support for fatal UTF-8 decoder in no-ICU version unless streaming is used
3. add utf-8 fast path to no-ICU version
4. reduce duplication of TextDecoder source
5. normalize how `this[kHandle]` was not set in no-ICU version
6. The refactor changes the encoding reported in the no-ICU version when encoding is recognized but unsupported: now the actual resolved encoding name is reported as unsupported when previously the original input string was reported.
I don't think this is major, and reporting the actual encoding name is better in that case.

This will allow further fixes and common fast path for UTF-16 / fixed impl for UTF-16 as string_decoder doesn't implement UTF-16 per spec.

UTF-16 is still broken in no-ICU version here, this PR does not address that.
Tracking: #61041 



Changed files:
filename: lib/internal/encoding.js
patch: @@ -23,7 +23,7 @@ const {
   ERR_INVALID_THIS,
   ERR_NO_ICU,
 } = require('internal/errors').codes;
-const kMethod = Symbol('method');
+const kSingleByte = Symbol('single-byte');
 const kHandle = Symbol('handle');
 const kFlags = Symbol('flags');
 const kEncoding = Symbol('encoding');
@@ -52,6 +52,8 @@ const {
   validateObject,
   kValidateObjectAllowObjectsAndNull,
 } = require('internal/validators');
+
+const { hasIntl } = internalBinding('config');
 const binding = internalBinding('encoding_binding');
 const {
   encodeInto,
@@ -405,166 +407,110 @@ function parseInput(input) {
   }
 }
 
-const TextDecoder =
-  internalBinding('config').hasIntl ?
-    makeTextDecoderICU() :
-    makeTextDecoderJS();
-
-function makeTextDecoderICU() {
-  const {
-    decode: _decode,
-    getConverter,
-  } = internalBinding('icu');
-
-  class TextDecoder {
-    constructor(encoding = 'utf-8', options = kEmptyObject) {
-      encoding = `${encoding}`;
-      validateObject(options, 'options', kValidateObjectAllowObjectsAndNull);
-
-      const enc = getEncodingFromLabel(encoding);
-      if (enc === undefined)
-        throw new ERR_ENCODING_NOT_SUPPORTED(encoding);
-
-      let flags = 0;
-      if (options !== null) {
-        flags |= options.fatal ? CONVERTER_FLAGS_FATAL : 0;
-        flags |= options.ignoreBOM ? CONVERTER_FLAGS_IGNORE_BOM : 0;
-      }
-
-      this[kDecoder] = true;
-      this[kFlags] = flags;
-      this[kEncoding] = enc;
-      this[kIgnoreBOM] = Boolean(options?.ignoreBOM);
-      this[kFatal] = Boolean(options?.fatal);
-      // Only support fast path for UTF-8.
-      this[kUTF8FastPath] = enc === 'utf-8';
-      this[kHandle] = undefined;
-      this[kMethod] = undefined;
-
-      if (isSinglebyteEncoding(this.encoding)) {
-        this[kMethod] = createSinglebyteDecoder(this.encoding, this[kFatal]);
-      } else if (!this[kUTF8FastPath]) {
-        this.#prepareConverter();
-      }
-    }
-
-    #prepareConverter() {
-      if (this[kHandle] !== undefined) return;
-      let icuEncoding = this[kEncoding];
-      if (icuEncoding === 'gbk') icuEncoding = 'gb18030'; // 10.1.1. GBK's decoder is gb18030's decoder
-      const handle = getConverter(icuEncoding, this[kFlags]);
-      if (handle === undefined)
-        throw new ERR_ENCODING_NOT_SUPPORTED(this[kEncoding]);
-      this[kHandle] = handle;
-    }
-
-    decode(input = empty, options = kEmptyObject) {
-      validateDecoder(this);
-      validateObject(options, 'options', kValidateObjectAllowObjectsAndNull);
+let icuDecode, icuGetConverter;
+if (hasIntl) {
+  ;({
+    decode: icuDecode,
+    getConverter: icuGetConverter,
+  } = internalBinding('icu'));
+}
 
-      if (this[kMethod]) return this[kMethod](parseInput(input));
+const kBOMSeen = Symbol('BOM seen');
 
-      this[kUTF8FastPath] &&= !(options?.stream);
+let StringDecoder;
+function lazyStringDecoder() {
+  if (StringDecoder === undefined)
+    ({ StringDecoder } = require('string_decoder'));
+  return StringDecoder;
+}
 
-      if (this[kUTF8FastPath]) {
-        return decodeUTF8(input, this[kIgnoreBOM], this[kFatal]);
-      }
+class TextDecoder {
+  constructor(encoding = 'utf-8', options = kEmptyObject) {
+    encoding = `${encoding}`;
+    validateObject(options, 'options', kValidateObjectAllowObjectsAndNull);
 
-      this.#prepareConverter();
+    const enc = getEncodingFromLabel(encoding);
+    if (enc === undefined)
+      throw new ERR_ENCODING_NOT_SUPPORTED(encoding);
 
-      let flags = 0;
-      if (options !== null)
-        flags |= options.stream ? 0 : CONVERTER_FLAGS_FLUSH;
+    let flags = 0;
+    if (options !== null) {
+      flags |= options.fatal ? CONVERTER_FLAGS_FATAL : 0;
+      flags |= options.ignoreBOM ? CONVERTER_FLAGS_IGNORE_BOM : 0;
+    }
 
-      return _decode(this[kHandle], input, flags, this.encoding);
+    this[kDecoder] = true;
+    this[kFlags] = flags;
+    this[kEncoding] = enc;
+    this[kIgnoreBOM] = Boolean(options?.ignoreBOM);
+    this[kFatal] = Boolean(options?.fatal);
+    this[kUTF8FastPath] = false;
+    this[kHandle] = undefined;
+    this[kSingleByte] = undefined; // Does not care about streaming or BOM
+
+    if (enc === 'utf-8') {
+      this[kUTF8FastPath] = true;
+    } else if (isSinglebyteEncoding(enc)) {
+      this[kSingleByte] = createSinglebyteDecoder(enc, this[kFatal]);
+    } else {
+      this.#prepareConverter(); // Need to throw early if we don't support the encoding
     }
   }
 
-  return TextDecoder;
-}
-
-function makeTextDecoderJS() {
-  let StringDecoder;
-  function lazyStringDecoder() {
-    if (StringDecoder === undefined)
-      ({ StringDecoder } = require('string_decoder'));
-    return StringDecoder;
+  #prepareConverter() {
+    if (this[kHandle] !== undefined) return;
+    if (hasIntl) {
+      let icuEncoding = this[kEncoding];
+      if (icuEncoding === 'gbk') icuEncoding = 'gb18030'; // 10.1.1. GBK's decoder is gb18030's decoder
+      const handle = icuGetConverter(icuEncoding, this[kFlags]);
+      if (handle === undefined)
+        throw new ERR_ENCODING_NOT_SUPPORTED(this[kEncoding]);
+      this[kHandle] = handle;
+    } else if (this[kEncoding] === 'utf-8' || this[kEncoding] === 'utf-16le') {
+      if (this[kFatal]) throw new ERR_NO_ICU('"fatal" option');
+      this[kHandle] = new (lazyStringDecoder())(this[kEncoding]);
+      this[kBOMSeen] = false;
+    } else {
+      throw new ERR_ENCODING_NOT_SUPPORTED(this[kEncoding]);
+    }
   }
 
-  const kBOMSeen = Symbol('BOM seen');
+  decode(input = empty, options = kEmptyObject) {
+    validateDecoder(this);
+    validateObject(options, 'options', kValidateObjectAllowObjectsAndNull);
 
-  function hasConverter(encoding) {
-    return encoding === 'utf-8' || encoding === 'utf-16le';
-  }
+    if (this[kSingleByte]) return this[kSingleByte](parseInput(input));
 
-  class TextDecoder {
-    constructor(encoding = 'utf-8', options = kEmptyObject) {
-      encoding = `${encoding}`;
-      validateObject(options, 'options', kValidateObjectAllowObjectsAndNull);
+    const stream = options?.stream;
+    if (this[kUTF8FastPath]) {
+      if (!stream) return decodeUTF8(input, this[kIgnoreBOM], this[kFatal]);
+      this[kUTF8FastPath] = false;
+    }
 
-      const enc = getEncodingFromLabel(encoding);
-      if (enc === undefined)
-        throw new ERR_ENCODING_NOT_SUPPORTED(encoding);
+    this.#prepareConverter();
 
-      let flags = 0;
-      if (options !== null) {
-        flags |= options.fatal ? CONVERTER_FLAGS_FATAL : 0;
-        flags |= options.ignoreBOM ? CONVERTER_FLAGS_IGNORE_BOM : 0;
-      }
-
-      this[kDecoder] = true;
-      this[kFlags] = flags;
-      this[kEncoding] = enc;
-      this[kIgnoreBOM] = Boolean(options?.ignoreBOM);
-      this[kFatal] = Boolean(options?.fatal);
-      this[kBOMSeen] = false;
-      this[kMethod] = undefined;
-
-      if (isSinglebyteEncoding(enc)) {
-        this[kMethod] = createSinglebyteDecoder(enc, this[kFatal]);
-      } else {
-        if (!hasConverter(enc)) throw new ERR_ENCODING_NOT_SUPPORTED(encoding);
-        if (this[kFatal]) throw new ERR_NO_ICU('"fatal" option');
-        // StringDecoder will normalize WHATWG encoding to Node.js encoding.
-        this[kHandle] = new (lazyStringDecoder())(enc);
-      }
+    if (hasIntl) {
+      const flags = stream ? 0 : CONVERTER_FLAGS_FLUSH;
+      return icuDecode(this[kHandle], input, flags, this[kEncoding]);
     }
 
-    decode(input = empty, options = kEmptyObject) {
-      validateDecoder(this);
-      input = parseInput(input);
-      validateObject(options, 'options', kValidateObjectAllowObjectsAndNull);
+    input = parseInput(input);
 
-      if (this[kMethod]) return this[kMethod](input);
-
-      if (this[kFlags] & CONVERTER_FLAGS_FLUSH) {
-        this[kBOMSeen] = false;
-      }
+    let result = stream ? this[kHandle].write(input) : this[kHandle].end(input);
 
-      if (options !== null && options.stream) {
-        this[kFlags] &= ~CONVERTER_FLAGS_FLUSH;
-      } else {
-        this[kFlags] |= CONVERTER_FLAGS_FLUSH;
+    if (result.length > 0 && !this[kBOMSeen] && !this[kIgnoreBOM]) {
+      // If the very first result in the stream is a BOM, and we are not
+      // explicitly told to ignore it, then we discard it.
+      if (result[0] === '\ufeff') {
+        result = StringPrototypeSlice(result, 1);
       }
+      this[kBOMSeen] = true;
+    }
 
-      let result = this[kFlags] & CONVERTER_FLAGS_FLUSH ?
-        this[kHandle].end(input) :
-        this[kHandle].write(input);
-
-      if (result.length > 0 && !this[kBOMSeen] && !this[kIgnoreBOM]) {
-        // If the very first result in the stream is a BOM, and we are not
-        // explicitly told to ignore it, then we discard it.
-        if (result[0] === '\ufeff') {
-          result = StringPrototypeSlice(result, 1);
-        }
-        this[kBOMSeen] = true;
-      }
+    if (!stream) this[kBOMSeen] = false;
 
-      return result;
-    }
+    return result;
   }
-
-  return TextDecoder;
 }
 
 // Mix in some shared properties.

filename: test/parallel/test-whatwg-encoding-custom-textdecoder.js
patch: @@ -67,32 +67,34 @@ assert(TextDecoder);
 }
 
 // Test TextDecoder, UTF-8, fatal: true, ignoreBOM: false
-if (common.hasIntl) {
-  ['unicode-1-1-utf-8', 'utf8', 'utf-8'].forEach((i) => {
-    const dec = new TextDecoder(i, { fatal: true });
-    assert.throws(() => dec.decode(buf.slice(0, 8)),
-                  {
-                    code: 'ERR_ENCODING_INVALID_ENCODED_DATA',
-                    name: 'TypeError',
-                    message: 'The encoded data was not valid ' +
-                          'for encoding utf-8'
-                  });
-  });
-
-  ['unicode-1-1-utf-8', 'utf8', 'utf-8'].forEach((i) => {
-    const dec = new TextDecoder(i, { fatal: true });
+['unicode-1-1-utf-8', 'utf8', 'utf-8'].forEach((i) => {
+  const dec = new TextDecoder(i, { fatal: true });
+  assert.throws(() => dec.decode(buf.slice(0, 8)),
+                {
+                  code: 'ERR_ENCODING_INVALID_ENCODED_DATA',
+                  name: 'TypeError',
+                  message: 'The encoded data was not valid ' +
+                        'for encoding utf-8'
+                });
+});
+
+['unicode-1-1-utf-8', 'utf8', 'utf-8'].forEach((i) => {
+  const dec = new TextDecoder(i, { fatal: true });
+  if (common.hasIntl) {
     dec.decode(buf.slice(0, 8), { stream: true });
     dec.decode(buf.slice(8));
-  });
-} else {
-  assert.throws(
-    () => new TextDecoder('utf-8', { fatal: true }),
-    {
-      code: 'ERR_NO_ICU',
-      name: 'TypeError',
-      message: '"fatal" option is not supported on Node.js compiled without ICU'
-    });
-}
+  } else {
+    assert.throws(
+      () => {
+        dec.decode(buf.slice(0, 8), { stream: true });
+      },
+      {
+        code: 'ERR_NO_ICU',
+        name: 'TypeError',
+        message: '"fatal" option is not supported on Node.js compiled without ICU'
+      });
+  }
+});
 
 // Test TextDecoder, label undefined, options null
 {
@@ -132,6 +134,7 @@ if (common.hasIntl) {
       '}'
     );
   } else {
+    dec.decode(Uint8Array.of(0), { stream: true });
     assert.strictEqual(
       util.inspect(dec, { showHidden: true }),
       'TextDecoder {\n' +