Document how to use StringDecoder in combination with stream.Transform

[mcollina]

In #7315 and #7425, it was discussed how to add string decoding capabilities to Writable. However, that is very tricky to implement without a performance regression. At the bare minimum, we should document this inside the streams API docs.

See https://github.com/mcollina/split2/blob/master/index.js as an example.

[chungngoops]

I would like to work on this one.

[code4cake]

is this issue still open? I would like to tackle it , but I see @chungngoops, you might me already working on it?

[chungngoops]

@dantesolis just go ahead, I'm working on it but it's ok.

[dicearr]

@mcollina I am digging into this issue and I would love to know if some of my conclusions are right before proposing a PR.

1. Major performance regressions comes due to multiple checks needed (type of chunk and encoding), rather than the decoding process.
2. The decoding process is tricky because of incomplete multibyte characters. They are not handled by StringDecoder.
3. Documentation should reflect previous conclusions and show an example of a custom Writable that decodes the Buffers when writing. It could be a new section inside implementing-a-writable-stream.

@chungngoops @dantesolis are you actively working on it?

[mcollina]

1. the major perf regression happens if we implement the decoding part within `Writable``
2. incomplete multibyte characters are handled within StringDecoder, and they must be flushed down in _final.
3. very likely yes ;).

[dicearr]

the major perf regression happens if we implement the decoding part within Writable

AFAIK this is due to the increasing number of checks that should be done in decodeChunk. At least that is what I have been able to conclude watching #7425.

very likely yes ;).

Something like this?

class StringWritable extends Writable {
  constructor (options) {
    super(options)
    const state = this._writableState
    this._decoder = new StringDecoder(state.defaultEncoding)
    this._data = ''
  }
  _write (chunk, encoding, callback) {
    if (encoding === 'buffer') {
      chunk = this._decoder.write(chunk)
    }
    this._data += chunk
    callback()
  }
}

[mcollina]

You are missing some data that might be left within the stringdecoder. Call this._decoder.end() to get it.

[jasnell]

I'd be curious to see the performance comparison when using util.TextDecoder instead. It is making use of the ICU converter which has historically been much more efficient.

[addaleax]

@jasnell util.TextDecoder is not a streaming decoder, unlike StringDecoder, and you really need to be careful when you’re comparing perf to take that into account :)

[jasnell]

@addaleax ... sure it is :-)

const { TextDecoder } = require('util');

class StringWritable extends Writable {
  constructor (options) {
    super(options)
    const state = this._writableState
    this._decoder = new TextDecoder()
    this._data = ''
  }

  _write (chunk, encoding, callback) {
    if (encoding === 'buffer') {
      chunk = this._decoder.decode(chunk, { stream: true })
    }
    this._data += chunk
    callback()
  }
}

See the stream option documented here: https://developer.mozilla.org/en-US/docs/Web/API/TextDecoder/decode

[addaleax]

@jasnell Oh, nice. In that case we might actually want to do away with StringDecoder at some point/merge the implementations?

[TimothyGu]

@addaleax FYI currently StringDecoder is actually used as a fallback for TextDecoder when ICU is disabled:

node/lib/internal/encoding.js

Lines 431 to 508 in f8063d5

	class TextDecoder {
	constructor(encoding = 'utf-8', options = {}) {
	if (!warned) {
	warned = true;
	process.emitWarning(experimental, 'ExperimentalWarning');
	}
	encoding = `${encoding}`;
	if (typeof options !== 'object')
	throw new errors.Error('ERR_INVALID_ARG_TYPE', 'options', 'object');
	const enc = getEncodingFromLabel(encoding);
	if (enc === undefined || !hasConverter(enc))
	throw new errors.RangeError('ERR_ENCODING_NOT_SUPPORTED', encoding);
	var flags = 0;
	if (options !== null) {
	if (options.fatal) {
	throw new errors.TypeError('ERR_NO_ICU', '"fatal" option');
	}
	flags |= options.ignoreBOM ? CONVERTER_FLAGS_IGNORE_BOM : 0;
	}
	this[kDecoder] = true;
	// StringDecoder will normalize WHATWG encoding to Node.js encoding.
	this[kHandle] = new (lazyStringDecoder())(enc);
	this[kFlags] = flags;
	this[kEncoding] = enc;
	this[kBOMSeen] = false;
	}
	decode(input = empty, options = {}) {
	if (this == null || this[kDecoder] !== true)
	throw new errors.TypeError('ERR_INVALID_THIS', 'TextDecoder');
	if (isArrayBuffer(input)) {
	input = lazyBuffer().from(input);
	} else if (isArrayBufferView(input)) {
	input = lazyBuffer().from(input.buffer, input.byteOffset,
	input.byteLength);
	} else {
	throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'input',
	['ArrayBuffer', 'ArrayBufferView']);
	}
	if (typeof options !== 'object') {
	throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'object');
	}
	if (this[kFlags] & CONVERTER_FLAGS_FLUSH) {
	this[kBOMSeen] = false;
	}
	if (options !== null && options.stream) {
	this[kFlags] &= ~CONVERTER_FLAGS_FLUSH;
	} else {
	this[kFlags] |= CONVERTER_FLAGS_FLUSH;
	}
	if (!this[kBOMSeen] && !(this[kFlags] & CONVERTER_FLAGS_IGNORE_BOM)) {
	if (this[kEncoding] === 'utf-8') {
	if (input.length >= 3 &&
	input[0] === 0xEF && input[1] === 0xBB && input[2] === 0xBF) {
	input = input.slice(3);
	}
	} else if (this[kEncoding] === 'utf-16le') {
	if (input.length >= 2 && input[0] === 0xFF && input[1] === 0xFE) {
	input = input.slice(2);
	}
	}
	this[kBOMSeen] = true;
	}
	if (this[kFlags] & CONVERTER_FLAGS_FLUSH) {
	return this[kHandle].end(input);
	}
	return this[kHandle].write(input);
	}
	}

[jasnell]

Can't do that entirely yet because TextDecoder does not support hex or base64