lib: port errors to new system #19034
Conversation
targos
added
semver-major
nodejs-github-bot
added
the
lib / src
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
The changes itself LG by briefly scanning over it but I personally would remove the BC.
There was a problem hiding this comment.
Is this not failing? I expected that the invalidArgTypeError call number has to be changed otherwise?
There was a problem hiding this comment.
Possible. It was failing because I forgot to update it, but then I forgot to run it again
There was a problem hiding this comment.
Without changing the errors that result in such changes, the PR would be a patch. I personally prefer a PR that is possible to backport instead of having the PR as semver-major because just very few cases that were wrong before.
There was a problem hiding this comment.
I can remove the semver-major changes, but I think a backport would have to be done from scratch anyway, even for v9.x, because many error codes don't exist in any release line
There was a problem hiding this comment.
Most errors were already ported in 9.x as far as I know. I personally still think it would be better to have this as a semver-patch but I am fine with it if others want to land it as is.
lib/module.js
Outdated
There was a problem hiding this comment.
To me it seems like this is a TypeError, not a RangeError. Don't you think so?
There was a problem hiding this comment.
The type is checked just before. I don't feel strongly about it but it seemed right to throw a RangeError: the must be greater than zero
There was a problem hiding this comment.
Well, I guess it is somewhat of a edge case. I do not feel strongly about it either.
There was a problem hiding this comment.
I think usually we just go with ERR_OUT_OF_RANGE for this sort of generic range errors..probably makes more sense if it's ERR_INVALID_ARG_VALUE.Error on id, or a ERR_OUT_OF_RANGE on id.length
There was a problem hiding this comment.
I changed it to a TypeError. @joyeecheung are you OK with that?
There was a problem hiding this comment.
Most errors were already ported in 9.x as far as I know. I personally still think it would be better to have this as a semver-patch but I am fine with it if others want to land it as is.
|
Rebased again. Hopefully last CI: https://ci.nodejs.org/job/node-test-pull-request/13484/ |
|
As a question: shall we squash this or land each on it's own? I am fine both ways, I just thought I ask because it is a lot of commits. |
|
I'm for squashing |
targos
added
the
author ready
|
And another CI: https://ci.nodejs.org/job/node-test-pull-request/13509/ There is no single green result but no failure seem related or reproducible so I'm going to land this. |
This is a first batch of updates that touches non-underscored modules in lib. PR-URL: #19034 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
|
Squashed and landed in 1e8d110 |
This is a first batch of updates that touches non-underscored modules in lib. PR-URL: nodejs#19034 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
5 participants
This is a first batch of updates that touches non-underscored modules in
/lib/.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
all subsystems :)
/cc @joyeecheung @BridgeAR @jasnell