-
-
Notifications
You must be signed in to change notification settings - Fork 31
Feat(node url to whatwg url)
#172
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
Conversation
This reverts commit c5ce549.
| @@ -0,0 +1,18 @@ | |||
| const url = require('node:url'); | |||
|
|
|||
| const myURL = url.parse('https://example.com/path?query=string#hash'); | |||
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.
Please add a test for parsing a relative url, for example:
url.parse('/path?query=string#hash')
JakobJingleheimer
left a comment
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.
Generally looks good! A couple things though:
- Dynamic imports?
- I think the
var→letneeds to get removed because it can easily break user code. (Requesting changes on this)
Also, I think Steven's request is a good one.
nit: I think indentation might be inconsistent across files? (spaces vs tabs)
PS Sorry it took me a bit to get to this 😓
|
update:
|
|
@AugustinMauroy is there a deprecation for this? |
This one is for https://nodejs.org/api/deprecations.html#DEP0116, I know because it’s described in the README. But I think it can handle https://nodejs.org/api/deprecations.html#DEP0169 as well |
| const myUrl = new URL('https://example.com/some/path?page=1&format=json').toString(); | ||
| ``` | ||
|
|
||
| > **Note:** The migration of `url.format` can also be done as `` `${new URL('https://example.com/some/path?page=1&format=json')}` `` which is little bit efficienter. |
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.
efficienter => more efficient
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 thought i already caught that 🤔
| @@ -0,0 +1,13 @@ | |||
| const url = require('node:url'); | |||
|
|
|||
| const myURL = new URL('/path?query=string#hash'); | |||
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.
This throws an error
Uncaught TypeError: Failed to construct 'URL': Invalid 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.
@AugustinMauroy This codemod is very dangerous because it will cause apps to crash at runtime.
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.
How so?
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.
@styfle if you can provide a better before/after example that can be used as test that will help us to fix this issue
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.
How so?
@JakobJingleheimer I'm not sure how else to explain. If you run the code it will throw:

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 you can provide a better before/after example
@AugustinMauroy The before is fine. The after is wrong. It should not transform the code at all. It needs to preserve the original url.parse('/path?query=string#hash')
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 created an issue to track this bug: #249
|
@AugustinMauroy FYI: there were some undocumented pieces to a recent related deprecation (DEP0169) that may affect this: nodejs/node#61644 |
Description
Add codemod for
node:urllegacy object to whatWG urlRelated issue
close #92