-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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, url: migrate to use internal/errors.js #11360
Conversation
lib/url.js
Outdated
@@ -92,7 +93,7 @@ function urlParse(url, parseQueryString, slashesDenoteHost) { | |||
|
|||
Url.prototype.parse = function(url, parseQueryString, slashesDenoteHost) { | |||
if (typeof url !== 'string') { | |||
throw new TypeError('Parameter "url" must be a string, not ' + typeof url); | |||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE','url','String',url); |
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.
Linter should complain here, there needs to be a space following the comma. Can you run make lint
to check this?
lib/url.js
Outdated
@@ -546,8 +547,7 @@ function urlFormat(obj, options) { | |||
if (typeof obj === 'string') { | |||
obj = urlParse(obj); | |||
} else if (typeof obj !== 'object' || obj === null) { | |||
throw new TypeError('Parameter "urlObj" must be an object, not ' + | |||
(obj === null ? 'null' : typeof obj)); | |||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE','obj','Object',obj); |
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.
Ditto.
@bougarfaoui Thanks so much for putting this together. Sorry that it is dragging out for so long due to being a semver-major change. Could you rebase and also squash your commits (I think all the changes should be one commit, right?). Thanks! |
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.
Are there any tests that cover these changes?
I'm closing this because it's been inactive for quite a while. Feel free to reopen or ping a collaborator to get it reopened if needed. |
I'll follow up |
The errors in |
Done |
Migrate url.js to use internal/errors.js
Refs: #11273
cc @jasnell .
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
errors, url