-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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.
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
errors, buffer: Migrate buffer errors to use internal/errors #13976
Conversation
lib/buffer.js
Outdated
'If encoding is specified then the first argument must be a string' | ||
); | ||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', { | ||
msg: 'The first argument must be of type string.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these should be passed parameters which provide info about the arg and expected type as opposed to a fixed string. See invalidArgType(name, expected, actual) in internal/errors.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
I think invalidArgType(name, expected, actual)
is unsuitable for all situations. Because "Invalid argument type" can caused by many reasons:
- Argument must be one or more specified type, but recieve other.
- Argument must NOT be one or more specified type, but recieve.
- In some specified condition, the above two happen.
But invalidArgType(name, expected, actual)
in internal/errors.js can only handle the first one, without concerning about the rest.
Should I extend the invalidArgType
method in order to make it suitable for all situations?
lib/buffer.js
Outdated
|
||
if (typeof value === 'number') | ||
throw new TypeError('"value" argument must not be a number'); | ||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
lib/buffer.js
Outdated
@@ -196,7 +200,9 @@ Buffer.from = function(value, encodingOrOffset, length) { | |||
length); | |||
} | |||
|
|||
throw new TypeError(kFromErrorMsg); | |||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
lib/buffer.js
Outdated
@@ -208,12 +214,14 @@ function assertSize(size) { | |||
let err = null; | |||
|
|||
if (typeof size !== 'number') { | |||
err = new TypeError('"size" argument must be a number'); | |||
// err = new TypeError('"size" argument must be a number'); | |||
err = new errors.TypeError('ERR_INVALID_ARG_TYPE', 'size', 'number', size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
lib/buffer.js
Outdated
throw new Error( | ||
'If encoding is specified then the first argument must be a string' | ||
); | ||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be passed arguments that provide info about the expected argument as opposed to a specific string. Look at the function associated with ERR_INVALID_ARG_TYPE in internal/errors.js. Same comment applies to everywhere ERR_INVALID_ARG_TYPE is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
lib/buffer.js
Outdated
} else if (size > binding.kMaxLength) { | ||
err = new RangeError('"size" argument must not be larger ' + | ||
'than ' + binding.kMaxLength); | ||
err = new errors.RangeError('ERR_VALUE_OUT_OF_RANGE', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a good candidate for adding a function that takes info about the arguments and the expected range as opposed to using a fixed string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
cd92a68
to
735590b
Compare
doc/api/errors.md
Outdated
@@ -586,6 +586,10 @@ Used when `Console` is instantiated without `stdout` stream or when `stdout` or | |||
|
|||
Used when the native call from `process.cpuUsage` cannot be processed properly. | |||
|
|||
<a id="ERR_DEPRECATED_METHOD"></a> | |||
### ERR_DEPRECATED_METHOD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid confusion, perhaps ERR_NO_LONGER_SUPPORTED
would be better here. The method itself is not deprecated, only one particular way of calling it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
07afe26
to
1778514
Compare
test/parallel/test-buffer-from.js
Outdated
@@ -34,8 +34,10 @@ deepStrictEqual(Buffer.from( | |||
runInNewContext('new String(checkString)', {checkString})), | |||
check); | |||
|
|||
const err = new RegExp('^TypeError: First argument must be a string, Buffer, ' + | |||
'ArrayBuffer, Array, or array-like object\\.$'); | |||
const err = new RegExp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated to be a common.expectsError()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
test/common/index.js
Outdated
@@ -56,7 +56,11 @@ exports.isOSX = process.platform === 'darwin'; | |||
|
|||
exports.enoughTestMem = os.totalmem() > 0x40000000; /* 1 Gb */ | |||
exports.bufferMaxSizeMsg = new RegExp( | |||
`^RangeError: "size" argument must not be larger than ${buffer.kMaxLength}$`); | |||
'RangeError \\[ERR_VALUE_OUT_OF_RANGE\\]: ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work is these were just strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works, but I'm updating it to use common.expectsError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
lib/internal/errors.js
Outdated
@@ -234,3 +248,22 @@ function oneOf(expected, thing) { | |||
return `of ${thing} ${String(expected)}`; | |||
} | |||
} | |||
|
|||
function valueOutOfRange(name, compareWord, target, actual) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this is not overkill... For example https://github.com/nodejs/node/blob/master/lib/readline.js#L109
Can you try and find any more places where it's useful?
Otherwise a simpler error message should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
doc/api/errors.md
Outdated
<a id="ERR_NO_LONGER_SUPPORTED"></a> | ||
### ERR_NO_LONGER_SUPPORTED | ||
|
||
Used when call a method which is no longer supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used when a Node.js API in called in an unsupported manner.
For example: `Buffer.write(string, encoding, offset[, length])`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
ed482e1
to
512cb57
Compare
lib/buffer.js
Outdated
@@ -1178,7 +1180,7 @@ Buffer.prototype.readDoubleBE = function readDoubleBE(offset, noAssert) { | |||
|
|||
function checkInt(buffer, value, offset, ext, max, min) { | |||
if (value > max || value < min) | |||
throw new TypeError('"value" argument is out of bounds'); | |||
throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'value', value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this error code is suitable.
Should we have a new error code for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should probably be a RangeError
Now all the errors in buffer module have been migrated into internal/errors. |
lib/buffer.js
Outdated
throw new Error( | ||
'If encoding is specified then the first argument must be a string' | ||
); | ||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'first argument', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the name of the argument as opposed to 'first argument'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sames goes for other similar instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is checking for the https://nodejs.org/api/buffer.html#buffer_new_buffer_string_encoding case first argument
-> string
|
||
if (typeof value === 'number') | ||
throw new TypeError('"value" argument must not be a number'); | ||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'value', 'not number', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the string end up being here ? I think this may be the first case were we have the 'not' cases versus listing out what it should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extended the invalidArgType(name, expected, actual)
in internal/errors.js to make it adapt to the 'not' cases. See here.
The case here will end up being:
TypeError [ERR_INVALID_ARG_TYPE]: The "value" argument must not be of type number. Received type number
The tests for it is also changed.
lib/buffer.js
Outdated
@@ -300,7 +299,7 @@ function fromString(string, encoding) { | |||
} else { | |||
length = byteLength(string, encoding, true); | |||
if (length === -1) | |||
throw new TypeError('"encoding" must be a valid string encoding'); | |||
throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'encoding', encoding); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should likely be 'string encoding'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other places UNKNOWN_ENCODING so that should be used here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but IMHO name should stay encoding
like in the doc - https://nodejs.org/api/buffer.html#buffer_new_buffer_string_encoding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think UNKNOWN_ENCODING is more suitable for this.
lib/buffer.js
Outdated
@@ -401,7 +400,8 @@ Buffer.isBuffer = function isBuffer(b) { | |||
|
|||
Buffer.compare = function compare(a, b) { | |||
if (!isUint8Array(a) || !isUint8Array(b)) { | |||
throw new TypeError('Arguments must be Buffers or Uint8Arrays'); | |||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'arguments', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not quite fit with the new approach. It really should be one of 'a' or 'b' instead of 'arguments'. I think we may need to different cases one that throws an error for 'a' and one for 'b' to be consistent with how we through errors everywhere else. To match that the second string passed in should be the name of the argument defined in the function definition that is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it should be buf1
and buf2
alike in the API docs https://nodejs.org/api/buffer.html#buffer_class_method_buffer_compare_buf1_buf2
lib/buffer.js
Outdated
@@ -635,8 +636,8 @@ Buffer.prototype.toString = function(encoding, start, end) { | |||
|
|||
Buffer.prototype.equals = function equals(b) { | |||
if (!isUint8Array(b)) | |||
throw new TypeError('Argument must be a Buffer or Uint8Array'); | |||
|
|||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'argument', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here as before, probably need to check for all cases that the second string matches the name of the argument that was invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This turns out to be one of the more challenging ones to convert, thanks for the work so far. A few more comments. |
doc/api/errors.md
Outdated
<a id="ERR_NO_LONGER_SUPPORTED"></a> | ||
### ERR_NO_LONGER_SUPPORTED | ||
|
||
Used when a Node.js API in called in an unsupported manner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, in
-> is
lib/buffer.js
Outdated
throw new Error( | ||
'If encoding is specified then the first argument must be a string' | ||
); | ||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'first argument', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is checking for the https://nodejs.org/api/buffer.html#buffer_new_buffer_string_encoding case first argument
-> string
lib/buffer.js
Outdated
@@ -300,7 +299,7 @@ function fromString(string, encoding) { | |||
} else { | |||
length = byteLength(string, encoding, true); | |||
if (length === -1) | |||
throw new TypeError('"encoding" must be a valid string encoding'); | |||
throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'encoding', encoding); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but IMHO name should stay encoding
like in the doc - https://nodejs.org/api/buffer.html#buffer_new_buffer_string_encoding
test/common/index.js
Outdated
exports.bufferMaxSizeMsg = expectsError({ | ||
code: 'ERR_INVALID_OPT_VALUE', | ||
type: RangeError, | ||
message: /^The value ".*" is invalid for option "size"$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you replace .*
with [^"]*
just to be more explicit
(and at L65 too)
Since common
is becoming a Winnebago
, I'm actually not in love with these two. The old one is reused 3 times, and the new one 2 times.
Maybe for your next PR you could inline them 😉
@starkwang this is really good work. I would approve after you address the comments. Optionally if you have some more patience to replace all the |
dd2bdc4
to
7c28ac2
Compare
Pushed commit to address comments. |
4a6cd45
to
ac9dc54
Compare
It seems like the CI failed after merged to the master. |
test/common/index.js
Outdated
@@ -717,7 +714,7 @@ exports.expectsError = function expectsError({code, type, message}) { | |||
} | |||
return true; | |||
}; | |||
}; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't lint:
not ok 3 - /usr/home/iojs/build/workspace/node-test-linter/test/common/index.js
---
message: Missing semicolon.
severity: error
data:
line: 717
column: 2
ruleId: semi
...
Simplest thing is to restore the previous code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I forget make lint
to check it.
Stupid mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what the CI is for
ac9dc54
to
4035559
Compare
4035559
to
bcde08c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Killed https://ci.nodejs.org/job/node-test-commit-arm/10847/nodes=armv7-wheezy/ after it had a few test fails, and there are others as well (12 in total):
|
bcde08c
to
0b2a6cb
Compare
Just help thing along I rebased and added the missing call count in the tests |
Full CI: https://ci.nodejs.org/job/node-test-pull-request/9095/ CI is ✔️ (except for known unrelated issues) |
PR-URL: nodejs#13976 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
0b2a6cb
to
dbfe8c4
Compare
@refack Thank you for your continued support! :D |
This project progresses by the efforts of contributors such as yourself (Collaborators are here mostly to support that)! |
Migrate buffer errors to use internal/errors.
Ref: #11273
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
buffer, errors