-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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, process: port internal/process errors to internal/errors #11294
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,4 +80,32 @@ module.exports = exports = { | |
// | ||
// Note: Please try to keep these in alphabetical order | ||
E('ERR_ASSERTION', (msg) => msg); | ||
E('ERR_INVALID_ARG_TYPE', invalidArgType); | ||
E('ERR_INVALID_CALLBACK', 'callback must be a function'); | ||
E('ERR_STDERR_CLOSE', 'process.stderr cannot be closed'); | ||
E('ERR_STDOUT_CLOSE', 'process.stdout cannot be closed'); | ||
E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type'); | ||
E('ERR_UNKNOWN_STREAM_TYPE', 'Unknown stream file type'); | ||
// Add new errors from here... | ||
|
||
function invalidArgType(name, expected, actual) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copying my comment from #11300: If I read it correctly, this would print |
||
assert(name, 'name is required'); | ||
assert(expected, 'expected is required'); | ||
var msg = `The "${name}" argument must be `; | ||
if (Array.isArray(expected)) { | ||
var len = expected.length; | ||
expected = expected.map((i) => String(i)); | ||
if (len > 1) { | ||
msg += `one of type ${expected.slice(0, len - 1).join(', ')}, or ` + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe break out the |
||
expected[len - 1]; | ||
} else { | ||
msg += `of type ${expected[0]}`; | ||
} | ||
} else { | ||
msg += `of type ${String(expected)}`; | ||
} | ||
if (arguments.length >= 3) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With only the first three arguments being used, maybe make this Feel free to disregard |
||
msg += `. Received type ${actual !== null ? typeof actual : 'null'}`; | ||
} | ||
return msg; | ||
} |
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.
(Not sure which PR is the right place to ask, this one seems the oldest so I'll ask here)
Which one does the "type" here cover:
typeof
types, doesn't differentiateDate
andRegExp
and stuff)Object.prototype.toString.call()
, differentiatesDate
andRegExp
)instanceof
types, differentiatesDate
andRegExp
)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.
Also, some PRs use the
typeof
types as theexpected
type(e.g. "function"), some uses the class name/type tag(e.g. "Object"), we probably need to be a little bit consistent on this one.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 not handled very consistently throughout the Node.js source. Sometimes the value itself is passed in, sometimes the typeof is used, etc. The goal here would be to start getting some consistency but it's hard to nail down exactly what that 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.
Got it, so we should focus on migrating the errors to use the new system with a reasonable
code
right now, and deal withmessage
later, because after the migration it would be easier to change them anyway?