internal/errors: improve invalid arg type #14057
Conversation
nodejs-github-bot
added
the
lib / src
|
/cc @mscdex @joyeecheung @mhdawson, this is a fresh clone of #13834 PTAL |
refack
added
dont-land-on-v4.x
errors
| received = String(actual); | ||
| } else if (typeof actual === 'object') { | ||
| if (actual.constructor != null) { | ||
| received = `instance of ${actual.constructor.name}`; |
There was a problem hiding this comment.
Provided that actual is an arbitrary user-provided object which we are validating, after this patch, exceptions can be raised on any of:
actual.constructoraccess.actual.constructor.nameaccess.- String concatenation with
actual.constructor.name(if it's aSymbolorString(actual.constructor.name)throws)
There was a problem hiding this comment.
This is an uncommon code path, so wrapping in try-catch should be fine?
There was a problem hiding this comment.
I do not agree that it's useful to guard against a constructor or name property set to a getter that throws or being set to a symbol.
If this would ever result in an error, nothing bad would happen besides showing the "wrong" error.
For the error type ERR_INVALID_ARG_TYPE the expected type should be lower cased in case it's a primitive value. The actual value should always be present as it increases the provided error message information and it should not be passed to the error type as is. In addition this fixes some typos in the arguments names.
The error type ERR_INVALID_ARG_TYPE receives a actual value. Make that mandatory so the error message has more detailed information of what went wrong.
BridgeAR
force-pushed
the
improve-invalid-arg-type
branch
from
455dd27
to
1717cb9
Compare
|
Rebased due to conflicts (the commits will have to be squashed as it would be really tedious to make each commit pass). |
|
Unfortunately this now needs a rebase again. Thanks for your patience. |
BridgeAR
added
blocked
|
Just as a note - I still plan on adding this but I decided to wait until all errors got ported. The reason was that it became really difficult to get this landed at that point of time due to lots of conflicting PRs. As we are almost through with the error codes, I am going to look into this soon again. |
|
@BridgeAR I think we are at a point where this PR can be revisited since most of the errors have now been ported |
|
@joyeecheung I would like to work on this again but I am not sure when I really get back to it. It is quite some significant work to refactor all those tests. Because of that reason I am closing the PR for now. I might get back to it again at another point. I definitely want to work more on improving our errors in general. |
Something went wrong with #13834
This is the original change as it should be.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
internal/errors