errors,http_server: migrate to use internal/errors.js #13301
Conversation
nodejs-github-bot
added
errors
jasnell
added
the
semver-major
lib/_http_server.js
Outdated
There was a problem hiding this comment.
Since we're updating anyway, it would be nice to get rid of the contraction.
Cannot render headers ...
There was a problem hiding this comment.
ERR_ASSERTION is the wrong choice here. That should be reserved for uses of the assert module. A new error code would be more appropriate.
There was a problem hiding this comment.
@jasnell Rectifications done. Thank You
lib/_http_server.js
Outdated
There was a problem hiding this comment.
Likewise, ERR_ASSERTION is the wrong choice here. A new error code would be better.
lib/internal/errors.js
Outdated
There was a problem hiding this comment.
Since this is specific to http, perhaps ERR_HTTP_INVALID_CHAR
lib/internal/errors.js
Outdated
There was a problem hiding this comment.
same here... ERR_HTTP_INVALID_STATUS_CODE
lib/internal/errors.js
Outdated
There was a problem hiding this comment.
Rectified. Thanks!
There was a problem hiding this comment.
This shouldn't need the --expose-internals flag
There was a problem hiding this comment.
Likewise, this shouldn't need the --expose-internals flag
lib/_http_server.js
Outdated
There was a problem hiding this comment.
I'm guessing its broken into 2 strings to avoid the long line. I'm not sure I've seen us use the + for this, instead could you just put the 'Invalid ...' on a second line
There was a problem hiding this comment.
Done as per suggestion. Thanks!
PR-URL: #13301 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Landed in a9f798e |
4 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
_http_server.js
internal/errors.js
ref: #11273
I read and understood the contribution guidelines, please review and suggest.
@jasnell
Thanks.