-
Notifications
You must be signed in to change notification settings - Fork 29k
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
lib: migrate errors to internal/errors #17719
Conversation
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.
Hi, thank you for opening the pull request, can you add a test for this new error?
lib/fs.js
Outdated
if (!(data instanceof Buffer)) { | ||
const err = new errors.TypeError('ERR_INVALID_ARG_TYPE', | ||
'data', | ||
['string', 'Buffer'], |
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.
The type check only checks if the data is a buffer, can you change this to 'Buffer'
instead?
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 was looking at the usage elsewhere in the same file and missed this. Thanks, I fixed it! 👍
@joyeecheung I created a test but I have no idea if it's in the correct place. |
lib/fs.js
Outdated
if (!(data instanceof Buffer)) { | ||
const err = new errors.TypeError('ERR_INVALID_ARG_TYPE', | ||
'data', | ||
['Buffer'], |
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.
Here can be reduced to use a single string 'Buffer'
, instead of an array ['Buffer']
: )
}, { | ||
code: 'ERR_INVALID_ARG_TYPE', | ||
type: TypeError, | ||
message: 'The "data" argument must be of type Buffer without null bytes.' + |
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.
Umm... If I'm not mistaken, the message here will not match the error it actually throws.
I think it should be 'The "data" argument must be of type Buffer. Received type 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.
Yes, that's probably why the test is failing, you just need to change this to match the actual error message.
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! 🔧
@@ -41,6 +41,19 @@ const filename = path.join(common.tmpDir, 'sync-write-stream.txt'); | |||
assert.strictEqual(fs.readFileSync(filename).equals(chunk), true); | |||
} | |||
|
|||
// Throws if data is not of type Buffer. | |||
{ | |||
const stream = new SyncWriteStream(1); |
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.
You can just get the stream using new fs.WriteStream
, or fs.createWriteStream
, SyncWriteStream
is being deprecated. That way this test should be in test-fs-write-stream.js
instead.
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.
Thanks! I made the changes.
I debugged the file to see why I was getting the error that the code
was expected ERR_INVALID_ARG_TYPE
but actual was undefined
.
It looks like require(fs
) is grabbing my installed 8.x fs
and not lib/fs.js
. Thoughts on this?
@@ -2379,8 +2379,13 @@ WriteStream.prototype.open = function() { | |||
|
|||
|
|||
WriteStream.prototype._write = function(data, encoding, cb) { | |||
if (!(data instanceof Buffer)) | |||
return this.emit('error', new Error('Invalid data')); | |||
if (!(data instanceof Buffer)) { |
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.
Should likely update this to use Buffer.isType()
but let's check with @mcollina first
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 would keep this as it is, with instanceof
. Buffer.isBuffer
is mainly needed outside of core.
@@ -2379,8 +2379,13 @@ WriteStream.prototype.open = function() { | |||
|
|||
|
|||
WriteStream.prototype._write = function(data, encoding, cb) { | |||
if (!(data instanceof Buffer)) | |||
return this.emit('error', new Error('Invalid data')); | |||
if (!(data instanceof Buffer)) { |
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 would keep this as it is, with instanceof
. Buffer.isBuffer
is mainly needed outside of core.
'data', | ||
'Buffer', | ||
data); | ||
return this.emit('error', err); |
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.
while we are at it, I would see if we can convert this to this.detroy(err)
instead.
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.
Hi @mcollina, this is my first time looking at the source code for Node so I'm not sure what the implications are of changing that line 🙃
- Do you want me to change
return this.emit('error', err);
toreturn this.detroy(err);
? - How would I ensure that change is working correctly (besides existing unit tests)?
Thanks! 😄
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.
-
yes.
-
run our unit tests. You can check if
_destroy
gets called, but that's a plus.
@joyeecheung Can you review again? Thanks! |
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
@styfle If you want to run the test with your changes, you can use the |
lib/fs.js
Outdated
'data', | ||
'Buffer', | ||
data); | ||
return this.detroy(err); |
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.
detroy
-> destroy
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.
@starkwang Oops 😬 That’s embarrassing.
I blame @mcollina because I copied the comment above verbatim 😜
In other news, I realized my MacBook wasn’t actually building because Xcode isn’t installed, but linting seems to work fine. 🤨
Let’s see if this one works 💯
lib/fs.js
Outdated
'data', | ||
'Buffer', | ||
data); | ||
return this.destroy(err); |
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.
The CI seems failed due to this line. It should be:
return this.emit('error', err);
I've tested it on my environment and passed all the test 😉
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 it was before but @mcollina said to use destroy instead.
I will change it back.
Thanks for the heads up 😎
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.
@starkwang How does that look?
This should EXTERMINATE the typo
All, thank you for coaching me through this PR! I hope everyone takes a break from working on node this week to enjoy time with friends and family! 😄 🎄 🎁 ☃️ |
Landed in 8599465, thanks! |
Refs: #17709
cc @jasnell @joyeecheung
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
lib, test