Skip to content

string_decoder : support typed array or data view #22562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
BeniCheni wants to merge 7 commits into from
Closed

string_decoder : support typed array or data view #22562

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8f14d9c
string_decoder: support typed array or data view
BeniCheni Aug 28, 2018
7505b48
string_decoder: update code format for ERR_INVALID_ARG_TYPE code area
BeniCheni Aug 28, 2018
3a25c6e
string_decoder: update source & tests for TypedArray & DataView per r…
BeniCheni Sep 1, 2018
9057575
doc: update .end & .write string_decoder.md
BeniCheni Sep 1, 2018
62393c0
test: update string-decoder tests w. ArrayBufferView
BeniCheni Sep 5, 2018
5fb3198
test: shorten test message in string-decoder tests w. ArrayBufferView
BeniCheni Sep 5, 2018
510ac79
test: remove console log message from string_decoder's ArrayBufferVie…
BeniCheni Sep 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
string_decoder: support typed array or data view 
Fixes: #1826
  • Loading branch information
@BeniCheni
BeniCheni committed Sep 11, 2018
commit 8f14d9c4a5eddfe0ba49c8fdad772a5513498ded
7 changes: 6 additions & 1 deletion lib/string_decoder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,12 @@ StringDecoder.prototype.write = function write(buf) {
return buf;
if (!ArrayBuffer.isView(buf))
throw new ERR_INVALID_ARG_TYPE('buf',
['Buffer', 'Uint8Array', 'ArrayBufferView'],
Copy link
Copy Markdown
Member

@Trott Trott Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is Uint8Array being removed here?

Copy link
Copy Markdown
Contributor Author

@BeniCheni BeniCheni Aug 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@addaleax addaleax Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

@SirR4T SirR4T Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. adding missing tests, for TypedArrays and DataViews wherever only Buffers or Uint8Arrays were tested earlier
  2. Updating code to work with these new types, where necessary(should mostly involve only JS side of things, in lib and maybe lib/internal)
  3. 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!

Copy link
Copy Markdown
Contributor Author

@BeniCheni BeniCheni Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[
'Buffer',
'TypedArray',
'DataView',
'ArrayBufferView'
],
buf);
return decode(this[kNativeDecoder], buf);
};
Expand Down
6 changes: 3 additions & 3 deletions test/parallel/test-string-decoder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])));
Copy link
Copy Markdown
Member

@Trott Trott Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Copy Markdown
Member

@Trott Trott Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@Trott Trott Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@BeniCheni BeniCheni Aug 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@devsnek devsnek Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert.strictEqual(decoder.lastNeed, 2);
assert.strictEqual(decoder.lastTotal, 3);

Expand Down Expand Up @@ -174,8 +174,8 @@ common.expectsError(
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "buf" argument must be one of type Buffer, Uint8Array, or' +
' ArrayBufferView. Received type object'
message: 'The "buf" argument must be one of type Buffer, TypedArray,' +
' DataView, or ArrayBufferView. Received type object'
}
);

Expand Down