-
Notifications
You must be signed in to change notification settings - Fork 633
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
feat(node): Add missing url.parse #1667
Conversation
node/url.ts
Outdated
} | ||
} | ||
|
||
const self = new URL(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.
I think url.parse
should return url.Url object, not a global URL object
You can check it in Node.js repl like the below
> url.parse("https://example.com") instanceof url.Url
true
> url.parse("https://example.com") instanceof URL
false
node/url.ts
Outdated
self.href = format(self); | ||
} |
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.
You forgot returning self
?
@getspooky |
@kt3k fixed , Now parse return url.Url object, not a global URL object. Deno: test [url] URL ...Url {
protocol: "http:",
slashes: true,
auth: null,
host: "github.com",
port: null,
hostname: "github.com",
hash: null,
search: null,
query: null,
pathname: "/",
path: "/",
href: "http://github.com/"
} Node: > url.parse("http://github.com")
Url {
protocol: 'http:',
slashes: true,
auth: null,
host: 'github.com',
port: null,
hostname: 'github.com',
hash: null,
search: null,
query: null,
pathname: '/',
path: '/',
href: 'http://github.com/'
} |
Thanks for updating! The change looks good to me, but the test file |
@kt3k done |
https://github.com/denoland/deno_std/runs/4410914506?check_suite_focus=true
We haven't implemented hasIntl yet, so it would be nice to comment it out like this. |
Co-authored-by: Yoshiya Hinosawa <[email protected]>
The assertion at line 985 seems failing. Maybe let's comment out it as well with a TODO comment. Also now you modified |
@kt3k fixed |
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.
@getspooky LGTM. Thank you for your contribution!
url.format
tests are still remaining, but this adds very accurate compatibility of url.parse
. Nice work!
Ref #1666