-
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: update url module to use module.exports = {} pattern #12717
Conversation
lib/url.js
Outdated
|
||
exports.Url = Url; | ||
// Legacy url.parse() API |
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.
Given we don't have an alternative for relative URLs yet, I'm not sure we should call it legacy in the source since there are use cases whatwg-url doesn't cover.
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.
Legacy definition is "denoting software or hardware that has been superseded but is difficult to replace because of its wide use." ... that would seem perfectly suitable for the url.parse()
API. Legacy does not imply that the use cases have been completely handled.
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.
Removed the word legacy
from this PR to keep that from holding up landing this.
PR-URL: #12717 Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com
PR-URL: #12717 Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com
Minor restructuring to
lib/url.js
to use the more efficientmodule.exports = {}
pattern.Also restructure binding imports in
lib/internal/url.js
to simplify code.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
url