node-url-to-whatwg-url codemod causes app to crash

[styfle]

My previous comment was not addressed so I'm opening a new issue to track this bug

• Brainstorm: url.parse() -> new url #92 (comment)

You can see the input gets transformed into the expected output. However the expected output crashes when you try to run it.

• node-url-to-whatwg-url/tests/url-parse/input/file-10.js
• node-url-to-whatwg-url/tests/url-parse/expected/file-10.js

Uncaught TypeError: Invalid URL
    at new URL (node:internal/url:825:25) {
  code: 'ERR_INVALID_URL',
  input: '/path?query=string#hash'

[AugustinMauroy]

so what should be the behavior In this case. I din't get how to fix that 😅

[styfle]

It should preserve the original code.

The file-10.js input should match the file-10.js output.

[AugustinMauroy]

So

1. Don't transform relative URL (so add logic to detect them). But there are remaining usage of a legacy API.
2. Document how to go out of this api in this case like using node:path

Did you have full use case of relative URL that you can share to us ?

[JakobJingleheimer]

The result of url.parse('/path?query=string#hash') is

Url {
  protocol: null,
  slashes: null,
  auth: null,
  host: null,
  port: null,
  hostname: null,
  hash: '#hash',
  search: '?query=string',
  query: 'query=string',
  pathname: '/path',
  path: '/path?query=string',
  href: '/path?query=string#hash'
}

Which is not something we can replicate with new URL: setting some transient value as the second param of URL() (like new URL('/path?query=string#hash', 'http://tmp')) handles the incomplete url issue, but then it's not really feasible to to remove those transient parts because the property setters don't accept empty values—these silently fail or behave unexpectedly/undesirably:

Object.assign(myURL, { hostname: '', protocol: '' }) // silently ignored
Object.assign(myURL, { hostname: null, protocol: null }) // `'null'`
Object.assign(myURL, { hostname: undefined, protocol: undefined }) // `'undefined'`
Object.assign(myURL, { hostname: void 0, protocol: void 0 }) // `'undefined'`

That probably won't cause a problem because users probably won't be accessing those properties anyway since they were previously empty.

I think the bigger problem is href (and toString()) which will consume them and return a wrong value.

Unless there is a way around that to strip out the transient property values, the next-best thing to do is collect the occurrences of url.parse(relativeUrl) an warn—something like:

⚠️ Cannot migrate `url.parse()` for relative urls because that is incompatible with the WHATWG standard `URL`. The following occurrences were ignored by this migration and must be fixed manually (see https://nodejs.org/api/deprecations.html#DEP0169 for options):

recipes/node-url-to-whatwg-url/tests/url-parse/input/file-10.cjs:3

[styfle]

the next-best thing to do is collect the occurrences of url.parse(relativeUrl) an warn

Sounds reasonable. We also have couple code snippets in the docs explaining how you might parse relative urls with WHATWG URL. So those could be suggested. But its hard for a codemod to know the full context.

https://nodejs.org/api/url.html#urlparseurlstring-parsequerystring-slashesdenotehost