Skip to content
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

Jump to bottom

util: extract out encoding validation functions #18421

Closed
wants to merge 5 commits into from
Closed

util: extract out encoding validation functions #18421

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f06ac13
Extract out encoding validation functions
timotew Jan 28, 2018
8f4a276
splitting out validateEncoder() and validateDecoder()
timotew Jan 29, 2018
b83472d
remove check for isArrayBufferView
timotew Jan 31, 2018
9ee5f96
split the buffer validation
timotew Jan 31, 2018
077c258
revert validate buffer
timotew Jan 31, 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
59 changes: 29 additions & 30 deletions lib/internal/encoding.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,21 @@ function lazyBuffer() {
return Buffer;
}

function validateEncoder(obj) {
if (obj == null || obj[kEncoder] !== true)
throw new errors.TypeError('ERR_INVALID_THIS', 'TextEncoder');
}

function validateDecoder(obj) {
if (obj == null || obj[kDecoder] !== true)
throw new errors.TypeError('ERR_INVALID_THIS', 'TextDecoder');
}

function validateArgument(prop, expected, propName) {
if (typeof prop !== expected && !isArrayBufferView(prop))
Copy link
Member

Choose a reason for hiding this comment

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

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.

throw new errors.Error('ERR_INVALID_ARG_TYPE', propName, expected);
}

const CONVERTER_FLAGS_FLUSH = 0x1;
const CONVERTER_FLAGS_FATAL = 0x2;
const CONVERTER_FLAGS_IGNORE_BOM = 0x4;
Expand Down Expand Up @@ -288,20 +303,17 @@ class TextEncoder {
}

get encoding() {
if (this == null || this[kEncoder] !== true)
throw new errors.TypeError('ERR_INVALID_THIS', 'TextEncoder');
validateEncoder(this);
return 'utf-8';
}

encode(input = '') {
if (this == null || this[kEncoder] !== true)
throw new errors.TypeError('ERR_INVALID_THIS', 'TextEncoder');
validateEncoder(this);
return encodeUtf8String(`${input}`);
}

[inspect](depth, opts) {
if (this == null || this[kEncoder] !== true)
throw new errors.TypeError('ERR_INVALID_THIS', 'TextEncoder');
validateEncoder(this);
if (typeof depth === 'number' && depth < 0)
return opts.stylize('[Object]', 'special');
var ctor = getConstructorOf(this);
Expand Down Expand Up @@ -329,8 +341,7 @@ const { hasConverter, TextDecoder } =
makeTextDecoderJS();

function hasTextDecoder(encoding = 'utf-8') {
if (typeof encoding !== 'string')
throw new errors.Error('ERR_INVALID_ARG_TYPE', 'encoding', 'string');
validateArgument(encoding, 'string', 'encoding');
return hasConverter(getEncodingFromLabel(encoding));
}

Expand All @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


const enc = getEncodingFromLabel(encoding);
if (enc === undefined)
Expand All @@ -369,17 +379,14 @@ function makeTextDecoderICU() {


decode(input = empty, options = {}) {
if (this == null || this[kDecoder] !== true)
throw new errors.TypeError('ERR_INVALID_THIS', 'TextDecoder');
validateDecoder(this);
if (isArrayBuffer(input)) {
input = lazyBuffer().from(input);
} else if (!isArrayBufferView(input)) {
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');
}
validateArgument(options, 'object', 'options');

var flags = 0;
if (options !== null)
Expand Down Expand Up @@ -416,8 +423,7 @@ function makeTextDecoderJS() {
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');

const enc = getEncodingFromLabel(encoding);
if (enc === undefined || !hasConverter(enc))
Expand All @@ -440,8 +446,7 @@ function makeTextDecoderJS() {
}

decode(input = empty, options = {}) {
if (this == null || this[kDecoder] !== true)
throw new errors.TypeError('ERR_INVALID_THIS', 'TextDecoder');
validateDecoder(this);
if (isArrayBuffer(input)) {
input = lazyBuffer().from(input);
} else if (isArrayBufferView(input)) {
Expand All @@ -451,9 +456,7 @@ function makeTextDecoderJS() {
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');
}
validateArgument(options, 'object', 'options');

if (this[kFlags] & CONVERTER_FLAGS_FLUSH) {
this[kBOMSeen] = false;
Expand Down Expand Up @@ -496,27 +499,23 @@ function makeTextDecoderJS() {
TextDecoder.prototype,
Object.getOwnPropertyDescriptors({
get encoding() {
if (this == null || this[kDecoder] !== true)
throw new errors.TypeError('ERR_INVALID_THIS', 'TextDecoder');
validateDecoder(this);
return this[kEncoding];
},

get fatal() {
if (this == null || this[kDecoder] !== true)
throw new errors.TypeError('ERR_INVALID_THIS', 'TextDecoder');
validateDecoder(this);
return (this[kFlags] & CONVERTER_FLAGS_FATAL) === CONVERTER_FLAGS_FATAL;
},

get ignoreBOM() {
if (this == null || this[kDecoder] !== true)
throw new errors.TypeError('ERR_INVALID_THIS', 'TextDecoder');
validateDecoder(this);
return (this[kFlags] & CONVERTER_FLAGS_IGNORE_BOM) ===
CONVERTER_FLAGS_IGNORE_BOM;
},

[inspect](depth, opts) {
if (this == null || this[kDecoder] !== true)
throw new errors.TypeError('ERR_INVALID_THIS', 'TextDecoder');
validateDecoder(this);
if (typeof depth === 'number' && depth < 0)
return opts.stylize('[Object]', 'special');
var ctor = getConstructorOf(this);
Expand Down