string_decoder : support typed array or data view #22562

BeniCheni · 2018-08-28T06:21:06Z

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

Fixes: #1826
Per this comment with the checklist, this PR attempts to help out the string_decoder scope from the check list. The PR supports explicitly TypedArray or DataView, insead of the original Uint8Array.

Trott · 2018-08-28T06:41:31Z

lib/string_decoder.js

-                                   buf);
+    throw new ERR_INVALID_ARG_TYPE(
+      'buf',
+      ['Buffer', 'TypedArray', 'DataView', 'ArrayBufferView'],


While we're in here, does it make sense to alphabetize the list? Or is the existing ordering logical for other reasons?

I think the list makes sense in that Buffer will be the most common type of argument (by far), followed by TypedArrays.

I am not sure why ArrayBufferView is listed explicitly, since currently there are (afaik) only TypedArray and DataView ABVs.

Trott · 2018-08-28T06:42:19Z

test/parallel/test-string-decoder.js

@@ -91,7 +91,7 @@ decoder = new StringDecoder('utf8');
 assert.strictEqual(decoder.write(Buffer.from('E1', 'hex')), '');

 // A quick test for lastChar, lastNeed & lastTotal which are undocumented.
-assert(decoder.lastChar.equals(new Uint8Array([0xe1, 0, 0, 0])));
+assert(decoder.lastChar.equals(Buffer.from([0xe1, 0, 0, 0])));


Why this change?

To elaborate a bit: There's nothing in this change that breaks new Uint8Array() working in this context, so why is the test case being changed instead of a new test case being added?

Shouldn't test cases be added for DataView and TypedArray? Do they even work properly? No code has been added in this PR to alter how they work with string_decoder. Were they working already or something?

I was working with the suggestion of this comment here:

[ ] string_decoder
This list is for TypedArray/DataView support only.

I was trying not to use Unit8Array. Hopefully I understood the #1826 correctly.

If this Unit8Array doesn't need to change to Buffer to align with others, I can revert this change here.

Buffer extends Uint8Array, I don't think this changed anything.

Trott · 2018-08-28T06:46:25Z

lib/string_decoder.js

@@ -72,9 +72,11 @@ StringDecoder.prototype.write = function write(buf) {
  if (typeof buf === 'string')
    return buf;
  if (!ArrayBuffer.isView(buf))
-    throw new ERR_INVALID_ARG_TYPE('buf',
-                                   ['Buffer', 'Uint8Array', 'ArrayBufferView'],


Why is Uint8Array being removed here?

From the suggestion of this comment in #1826, "string_decoder" seems to be on the list TypedArray/DataView support only. After viewing the fs PR and the child_process, I just gave my best attempt to follow the "idea" to handle string_decode.

Please let me know if I misunderstood too much off from the context of the checklist & goals from the comment.

I think this is okay and somewhat consistent with how we handle it elsewhere

Hi @BeniCheni , I think broadly we can categorize the changes for these PRs into:

adding missing tests, for TypedArrays and DataViews wherever only Buffers or Uint8Arrays were tested earlier

Updating code to work with these new types, where necessary(should mostly involve only JS side of things, in lib and maybe lib/internal)

Updating documentation to reflect these changes.

Here, your PR is updating the errors being thrown, but is neither adding test cases nor adding functionality to work with the TypedArrays or DataViews. Please note, it might well be the case that string_decoder already works for these, but adding test cases is a way of validation for the same. Hope that helps!

🙏 for the collective reviews. Will analyze & update accordingly.

BridgeAR

LGTM when ArrayBufferView is removed.

BeniCheni · 2018-09-01T04:31:32Z

@ Trott: Shouldn't test cases be added for DataView and TypedArray?
@ SirR4T: adding missing tests, for TypedArrays and DataViews

Researched & wrote tests to cover TypedArray & DataView.

@ Trott: Why is Uint8Array being removed here?
@ devsnek: Buffer extends Uint8Array, I don't think this changed anything.

Researched more and updated to revert the previously incorrect removal of Uint8Array.

@ addaleax: I am not sure why ArrayBufferView is listed explicitly,
@ BridgeAR: LGTM when ArrayBufferView is removed.

Removed any context of ArrayBufferView in string_decoder source per suggestion.

Also confirmed if any context needed to update in string_decoder.md doc, but didn't find any necessary edit. Please suggest if I miss anything in the string_decoder.md doc. Please review again.

BridgeAR

Reconfirming my LG

SirR4T · 2018-09-01T11:16:08Z

Hi @BeniCheni , thanks for updating the tests! I think we could also update string_decoder.md, specifically for the .end() and the .write() function documentations, since they accept not just buffers, but other types too.

BeniCheni · 2018-09-01T15:23:49Z

Hi @SirR4T, updated doc per your suggestion.

Notice the last CI instance failed with just a doc change in the commit. Awaiting to get help/advice from other reviewers to validate the failed CI instance.

Thoughts, any reviewer(s)?

SirR4T · 2018-09-01T15:37:54Z

Hi @BeniCheni , doc changes look good to me, thanks!

But as I'm myself a new enough contributor here, I'll refrain from approving PRs, in deference to someone with more review experience.

As for the CI failure, it doesn't seem related to your changes, but instead looks like a certificate failure from npm. This may or may not be related to npm's current issue at the time of this post.

addaleax · 2018-09-02T12:19:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16942/

addaleax · 2018-09-02T15:34:55Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/16953/

addaleax · 2018-09-02T19:02:12Z

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16971/ (:heavy_check_mark:)

TimothyGu · 2018-09-05T01:00:54Z

test/parallel/test-string-decoder.js

@@ -97,6 +97,21 @@ assert.strictEqual(decoder.lastTotal, 3);

 assert.strictEqual(decoder.end(), '\ufffd');

+// TypedArray with Int8Array test
+decoder = new StringDecoder('utf8');
+assert.strictEqual(decoder.write(new Int8Array([1, 2])), '\u0001\u0002');


I'd prefer this test use getArrayBufferViews() to cover all possible TypedArray types and DataView. An example usage of this function by @SirR4T is

node/test/parallel/test-fs-write-file-typedarrays.js

Lines 24 to 31 in be5e396

for (const expectView of common.getArrayBufferViews(inputBuffer)) {

console.log('Sync test for ', expectView[Symbol.toStringTag]);

fs.writeFileSync(filename, expectView);

assert.strictEqual(

fs.readFileSync(filename, 'utf8'),

inputBuffer.toString('utf8')

);

}

BeniCheni · 2018-09-05T03:21:53Z

@ TimothyGu:
I'd prefer this test use getArrayBufferViews() to cover all possible TypedArray types and DataView.

Hi, @TimothyGu. Updated to use the common getArrayBufferViews util to cover possible types. Please review at convenience? Thanks.

jasnell · 2018-09-10T16:40:39Z

test/parallel/test-string-decoder.js

+const arrayBufferViewStr = 'String for ArrayBufferView tests\n';
+const inputBuffer = Buffer.from(arrayBufferViewStr.repeat(8), 'utf8');
+for (const expectView of common.getArrayBufferViews(inputBuffer)) {
+  console.log(


If the console.log() statement is part of the test, please add a comment indicating such. If it is not, I'd prefer to use a comment instead of having the extraneous console output.

Fixes: #1826

…eviews Fixes: #1826

Refs: #22562 (comment)

…w tests Refs: #22562 (comment)

BeniCheni · 2018-09-11T04:14:40Z

@ jasnell
If the console.log() statement is part of the test, please add a comment indicating such. If it is not, I'd prefer to use a comment instead of having the extraneous console output.

Hello @jasnell,
I've checked & removed the unneeded console log message from the string_decode test code. Please review at convenience. Thanks.

BeniCheni · 2018-09-14T12:43:34Z

Ping/Dear reviewers,

Would you mind helping to guide this “author ready” PR to landing?

(Made all suggested updates according to previous review comments; thanks!)

addaleax · 2018-09-14T15:38:48Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/17187/

addaleax · 2018-09-17T15:49:49Z

Landed in e2325bc

Refs: #1826 PR-URL: #22562 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

nodejs-github-bot added the string_decoder Issues and PRs related to the string_decoder subsystem. label Aug 28, 2018

BeniCheni changed the title ~~String decoder typed array data view~~ string_decoder : support typed array or data view Aug 28, 2018

BeniCheni mentioned this pull request Aug 28, 2018

Accept ArrayBuffer (and typed array/data view?) anywhere Buffer is allowed in the API #1826

Closed

Trott reviewed Aug 28, 2018

View reviewed changes

BridgeAR approved these changes Aug 31, 2018

View reviewed changes

BridgeAR approved these changes Sep 1, 2018

View reviewed changes

addaleax approved these changes Sep 2, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 2, 2018

trivikr approved these changes Sep 4, 2018

View reviewed changes

TimothyGu reviewed Sep 5, 2018

View reviewed changes

jasnell reviewed Sep 10, 2018

View reviewed changes

BeniCheni added 7 commits September 10, 2018 23:48

string_decoder: support typed array or data view

8f14d9c

Fixes: #1826

string_decoder: update code format for ERR_INVALID_ARG_TYPE code area

7505b48

Fixes: #1826

string_decoder: update source & tests for TypedArray & DataView per r…

3a25c6e

…eviews Fixes: #1826

doc: update .end & .write string_decoder.md

9057575

Refs: #22562 (comment)

test: update string-decoder tests w. ArrayBufferView

62393c0

test: shorten test message in string-decoder tests w. ArrayBufferView

5fb3198

test: remove console log message from string_decoder's ArrayBufferVie…

510ac79

…w tests Refs: #22562 (comment)

addaleax closed this Sep 17, 2018

BeniCheni deleted the string-decoder-typed-array-data-view branch September 18, 2018 02:56

targos mentioned this pull request Sep 18, 2018

Release proposal: v10.11.0 #22932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

string_decoder : support typed array or data view #22562

string_decoder : support typed array or data view #22562

BeniCheni commented Aug 28, 2018 •

edited

Loading

Trott Aug 28, 2018

addaleax Aug 28, 2018

Trott Aug 28, 2018

Trott Aug 28, 2018

Trott Aug 28, 2018

BeniCheni Aug 28, 2018 •

edited

Loading

devsnek Aug 31, 2018

Trott Aug 28, 2018

BeniCheni Aug 28, 2018 •

edited

Loading

addaleax Aug 28, 2018

SirR4T Aug 28, 2018

BeniCheni Aug 28, 2018

BridgeAR left a comment

BeniCheni commented Sep 1, 2018 •

edited

Loading

BridgeAR left a comment •

edited

Loading

SirR4T commented Sep 1, 2018

BeniCheni commented Sep 1, 2018 •

edited

Loading

SirR4T commented Sep 1, 2018

addaleax commented Sep 2, 2018

addaleax commented Sep 2, 2018

addaleax commented Sep 2, 2018 •

edited

Loading

TimothyGu Sep 5, 2018

BeniCheni commented Sep 5, 2018 •

edited

Loading

jasnell Sep 10, 2018

BeniCheni commented Sep 11, 2018

BeniCheni commented Sep 14, 2018

addaleax commented Sep 14, 2018

addaleax commented Sep 17, 2018

	for (const expectView of common.getArrayBufferViews(inputBuffer)) {
	console.log('Sync test for ', expectView[Symbol.toStringTag]);
	fs.writeFileSync(filename, expectView);
	assert.strictEqual(
	fs.readFileSync(filename, 'utf8'),
	inputBuffer.toString('utf8')
	);
	}

string_decoder : support typed array or data view #22562

string_decoder : support typed array or data view #22562

Conversation

BeniCheni commented Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeniCheni Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeniCheni Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

BeniCheni commented Sep 1, 2018 • edited Loading

BridgeAR left a comment • edited Loading

Choose a reason for hiding this comment

SirR4T commented Sep 1, 2018

BeniCheni commented Sep 1, 2018 • edited Loading

SirR4T commented Sep 1, 2018

addaleax commented Sep 2, 2018

addaleax commented Sep 2, 2018

addaleax commented Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

BeniCheni commented Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

BeniCheni commented Sep 11, 2018

BeniCheni commented Sep 14, 2018

addaleax commented Sep 14, 2018

addaleax commented Sep 17, 2018

BeniCheni commented Aug 28, 2018 •

edited

Loading

BeniCheni Aug 28, 2018 •

edited

Loading

BeniCheni Aug 28, 2018 •

edited

Loading

BeniCheni commented Sep 1, 2018 •

edited

Loading

BridgeAR left a comment •

edited

Loading

BeniCheni commented Sep 1, 2018 •

edited

Loading

addaleax commented Sep 2, 2018 •

edited

Loading

BeniCheni commented Sep 5, 2018 •

edited

Loading