util: extract out encoding validation functions #18421

timotew · 2018-01-28T18:34:31Z

I saw some duplicate validations within internal/encoding.js, so I extracted them out to separate functions.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

util

cjihrig

If we go this route, I think validateEnDecoder() should be split into two separate functions.

jasnell · 2018-01-29T17:12:34Z

+1 to splitting out validateEncoder() and validateDecoder()

BridgeAR

Please do not change the validation or the error messages. Otherwise LG

BridgeAR · 2018-01-31T00:06:49Z

lib/internal/encoding.js

+}
+
+function validateArgument(prop, expected, propName) {
+  if (typeof prop !== expected && !isArrayBufferView(prop))


This is not only a extraction but it changes the validation itself as it also validates that the input is not an ArrayBufferView. I would like to keep it as is.

BridgeAR · 2018-01-31T00:09:19Z

lib/internal/encoding.js

@@ -344,8 +355,7 @@ function makeTextDecoderICU() {
  class TextDecoder {
    constructor(encoding = 'utf-8', options = {}) {
      encoding = `${encoding}`;
-      if (typeof options !== 'object')
-        throw new errors.Error('ERR_INVALID_ARG_TYPE', 'options', 'Object');
+      validateArgument(options, 'object', 'options');


By combining the check for the type with the second ERR_INVALID_ARG_TYPE argument the error message gets changed. That should not be the case. That applies to all validateArgument calls.

@BridgeAR the string type checks error messages is not affected.

BridgeAR · 2018-01-31T09:20:06Z

lib/internal/encoding.js

+}
+
+function validateArgument(prop, expected, propName) {
+  if (typeof prop !== expected.toLowerCase())


Using toLowerCase is a performance overhead.

I suggest to either split the validation in two functions or to just keep these validations as they were. You can of course also add a fourth argument.

@BridgeAR is this last commit inline with your suggestion?

BridgeAR · 2018-01-31T10:21:33Z

lib/internal/encoding.js

-      if (isArrayBuffer(input)) {
+      validateArgumentBuffer(input, 'input');
+
+      if (isArrayBuffer(input))


This results in a second isArrayBuffer check and is definitely not desirable. Please keep that check as it was before (as in: remove validateArgumentBuffer again).

BridgeAR · 2018-01-31T10:37:20Z

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

PR-URL: nodejs#18421 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR · 2018-02-01T10:55:45Z

Landed in b0b2045

I fixed the commit message while landing (adding the subsystem & shortening it)

PR-URL: #18421 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins · 2018-03-20T16:01:15Z

Should this be backported to v8.x-staging or v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

PR-URL: nodejs#18421 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Extract out encoding validation functions

f06ac13

nodejs-github-bot added the encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. label Jan 28, 2018

timotew changed the title ~~Extract out encoding validation functions~~ fs: extract out encoding validation functions Jan 28, 2018

timotew changed the title ~~fs: extract out encoding validation functions~~ util: extract out encoding validation functions Jan 28, 2018

cjihrig reviewed Jan 28, 2018

View reviewed changes

splitting out validateEncoder() and validateDecoder()

8f4a276

jasnell approved these changes Jan 29, 2018

View reviewed changes

BridgeAR reviewed Jan 31, 2018

View reviewed changes

remove check for isArrayBufferView

b83472d

BridgeAR reviewed Jan 31, 2018

View reviewed changes

split the buffer validation

9ee5f96

BridgeAR reviewed Jan 31, 2018

View reviewed changes

revert validate buffer

077c258

BridgeAR approved these changes Jan 31, 2018

View reviewed changes

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

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 1, 2018

lib: extract validation functions

b0b2045

PR-URL: nodejs#18421 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR closed this Feb 1, 2018

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

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

lib: extract validation functions

0429820

PR-URL: #18421 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

lib: extract validation functions

d93bad8

PR-URL: #18421 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

lib: extract validation functions

650ec2d

PR-URL: #18421 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added backport-requested-v6.x labels Mar 20, 2018

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

lib: extract validation functions

397f316

PR-URL: nodejs#18421 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: extract out encoding validation functions #18421

util: extract out encoding validation functions #18421

timotew commented Jan 28, 2018

cjihrig left a comment

jasnell commented Jan 29, 2018

BridgeAR left a comment

BridgeAR Jan 31, 2018

BridgeAR Jan 31, 2018

timotew Jan 31, 2018

BridgeAR Jan 31, 2018

timotew Jan 31, 2018

BridgeAR Jan 31, 2018

timotew Jan 31, 2018

BridgeAR commented Jan 31, 2018

BridgeAR commented Feb 1, 2018

MylesBorins commented Mar 20, 2018

util: extract out encoding validation functions #18421

util: extract out encoding validation functions #18421

Conversation

timotew commented Jan 28, 2018

Checklist

Affected core subsystem(s)

cjihrig left a comment

Choose a reason for hiding this comment

jasnell commented Jan 29, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Jan 31, 2018

Choose a reason for hiding this comment

BridgeAR Jan 31, 2018

Choose a reason for hiding this comment

timotew Jan 31, 2018

Choose a reason for hiding this comment

BridgeAR Jan 31, 2018

Choose a reason for hiding this comment

timotew Jan 31, 2018

Choose a reason for hiding this comment

BridgeAR Jan 31, 2018

Choose a reason for hiding this comment

timotew Jan 31, 2018

Choose a reason for hiding this comment

BridgeAR commented Jan 31, 2018

BridgeAR commented Feb 1, 2018

MylesBorins commented Mar 20, 2018