-
Notifications
You must be signed in to change notification settings - Fork 30k
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
url: stricter domainTo*() argument checking #12134
Conversation
@@ -1308,11 +1308,17 @@ function originFor(url, base) { | |||
} | |||
|
|||
function domainToASCII(domain) { | |||
if (arguments.length < 1) |
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.
Can you change these to checks against domain
explicitly. No need to introduce arguments
for no reason.
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.
That would be against the semantics used by other WHATWG URL interface methods. Why the aversion to arguments
in this case? arguments.length
is perfectly fine and optimizable by V8.
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.
Since url.host = undefined
actually sets the domain to "undefined"...it makes sense for domainTo*
too so arguments.length
is needed
Rationale for arguments.length
has been explained.
ee60063
to
03a3909
Compare
Rebased. New CI: https://ci.nodejs.org/job/node-test-pull-request/7185/ |
PR-URL: #12134 Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in c4469c4 |
cc @TimothyGu |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
url