-
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: stop exporting originFor() #10955
Conversation
I'm -1 on the full range of changes here. For now we can simply not export originFor in lib/url.js. |
fe27a9a
to
58cc1f4
Compare
@jasnell, changed. |
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.
If origin.inspect
is no longer tested then maybe origin.inspect
can be removed?
@joyeecheung, that was what I had at first (along with a change that removed unnecessary symbols), but @jasnell objected to it. |
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.
Not sure if we should leave untested code floating around, but LGTM anyway.
I'd like it to remain for now because I may have use for it. I need to finish up a few other things before I'll get back to that, however, so I'd like for it to stay in the meantime |
I'll land this tomorrow if there are no objections. |
@TimothyGu I think probably you need to consult @jasnell first whether he doesn't need this anymore? (from #10955 (comment)) |
@joyeecheung, I believe he was not referring to the |
@TimothyGu Oh right, I was confused about the "it"..in that case this is ready to land! Sorry about the confusion. |
58cc1f4
to
fa13ca8
Compare
Landed in e71c278. |
PR-URL: nodejs#10955 Fixes: nodejs#10800 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
In #10800, the utility of
url.originFor()
was discussed, and it was tentatively established that the function is not as useful as it might appear at first.I'm still open to more discussions either on that issue or here, and this PR is just created to get things moving and to show how a removal can look like.
Fixes: #10800
/cc @nodejs/url
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
url