src, os, dgram, tty: migrate to internal errors#16567
src, os, dgram, tty: migrate to internal errors#16567jasnell wants to merge 4 commits intonodejs:masterfrom
Conversation
|
ping @nodejs/tsc |
|
That wouldn't catch cases like |
|
Can't that case be changed to pass |
lib/internal/errors.js
Outdated
There was a problem hiding this comment.
context is guaranteed to be truthy here
|
This is really great! Should any docs be added on the C++ side about the new way to throw errors... maybe in the style guide? |
potentially. Let's save that particular change for later tho. Once I get through converting all of the native level code to use this I will verify that we can safely make that assumption.
Yes, I'm planning to write up a more detailed guide on this once I'm through the initial conversion. |
6350e83 to
4a8cbd3
Compare
doc/api/errors.md
Outdated
There was a problem hiding this comment.
Come to think of it, I think a more popular pattern in the userland is using the err.data property (correct me if I am wrong), I remember seeing some logging services/tools collect that automatically in addition to err.code...I remember there is another PR for crypto that use info. Is there a reason that we pick info instead of data?
There was a problem hiding this comment.
A quick search in our own code base shows that eslint is using error.data: https://github.com/nodejs/node/blob/master/tools/eslint/lib/config/config-validator.js#L92 and https://github.com/nodejs/node/blob/master/tools/eslint/lib/config/config-validator.js#L179
There was a problem hiding this comment.
I've seen both: https://www.npmjs.com/package/verror uses *.info
There was a problem hiding this comment.
On a side note: would be easier to read if we have a syntax for documenting optional properties..
There was a problem hiding this comment.
I agree, definitely something worth working on
joyeecheung
left a comment
There was a problem hiding this comment.
Changes LGTM, left a few suggestions.
lib/dgram.js
Outdated
There was a problem hiding this comment.
Is this for avoiding changing the error type? Maybe a comment because this looks a bit odd at first?
There was a problem hiding this comment.
Yes, I really dislike how this was done, but I want to save it for a different PR to change it.
lib/os.js
Outdated
There was a problem hiding this comment.
Maybe name this function and do a captureStackTrace()?
src/node_os.cc
Outdated
There was a problem hiding this comment.
For clarity, maybe use args[args.Length() - 1]? Also we should probably CHECK_EQ(args.Length(), n) or CHECK_GE(args.Length(), n) (not sure if these are used in userland)
There was a problem hiding this comment.
This one is not legacy, right? Should we move the comment below this?
Preparing for the migration of existing UVException and ErrnoExceptions from the native layer, add new `errors.SystemError` to internal/errors and new `env->CollectExceptionInfo()` / `env->CollectUVExceptionInfo()` methods.
4a8cbd3 to
ee9e7a6
Compare
|
@targos @joyeecheung ... updated, PTAL |
mhdawson
left a comment
There was a problem hiding this comment.
LGTM as long as CI is green.
|
CI is good. failures are unrelated and known flakies |
|
(getting this landed) |
Preparing for the migration of existing UVException and ErrnoExceptions from the native layer, add new `errors.SystemError` to internal/errors and new `env->CollectExceptionInfo()` / `env->CollectUVExceptionInfo()` methods. PR-URL: #16567 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #16567 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #16567 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #16567 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
src, os, dgram, tty