url: expose urlToOptions utility

[ZYSzys]

TBH, the urlToOptions utility is indeed useful for library developers, so expose it.

Fixes: #34349

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[Trott]

If we're going to expose this, I wonder if it's worth it to consider the name a bit. urlToOptions() isn't a bad name, but is there a better one?

[ZYSzys]

If we're going to expose this, I wonder if it's worth it to consider the name a bit. urlToOptions() isn't a bad name, but is there a better one?

So, any suggestions are welcome @Trott :)

[guybedford]

Can you share a little more details on the use case for this feature? I followed the link in the original issue, but it just seemed to be returning an options object - I couldn't tell what the goal was in having this options object available over the properties of URL directly?

[lpinca]

@guybedford it takes a URL and returns an options object that can be used for http.request(). For example auth, and path are not URL attributes.

Btw I agree with #34349 (comment).

[ZYSzys]

@guybedford I think @szmarczak has said the benefits clearly in the original issue #34349 (comment)

FYI, we can see there are millions of downloads of package url-to-options, which
means plenty of developers would benefit from this urlToOptions utility if we expose it.

[szmarczak]

If we're going to expose this, I wonder if it's worth it to consider the name a bit. urlToOptions() isn't a bad name, but is there a better one?

urlToHttpOptions()

[lpinca]

@ZYSzys @szmarczak what's wrong with having it as a userland module? It's not something hard to implement that requires access to Node.js core internals. Having it in as a npm package also makes it a lot easier to introduce breaking changes if/when needed.

[szmarczak]

what's wrong with having it as a userland module?

1. Increases package size.
2. If urlToOptions() changes inside the Node.js library, then the NPM module must be updated ASAP because the object is passed to http.request() and it expects us to pass an object returned by the Node.js library, not the NPM package.
3. Maintaining an NPM module in this case is just unnecessary overhead. And you have to rely on someone else - in case they decide to drop support what are we supposed to do?

It's trivial in the sense that it doesn't do anything that ordinary JS code cannot also do.

4. Then let's remove the http module completely since it doesn't do anything that an ordinary Node.js code cannot also do.

The point is that http is widely used, the same in case of urlToOptions().

it doesn't really fit anywhere (cross-cut of url and http)

5. It's meant for use by the http module, so http should export it. Eventually util.

it's annoying to have to worry about backwards compatibility / not breaking the public API when making changes

6. That's the cost you always take.

The distinction here is that it's only indirectly observable to user code.

Because it's almost not observable to user code it doesn't mean that it doesn't have any impact. If urlToOptions fails then you can clearly see the issue here.

Although the end user won't have to use urlToOptions, many packages that operate on the low level, including got , cacheable-request and caw use that.

There's already an NPM package called url-to-options.

Even node-fetch would benefit from this.

On the other side, request (now deprecated) and axios use the legacy url.parse instead.

[lpinca]

2. If urlToOptions() changes inside the Node.js library, then the NPM module must be updated ASAP because the object is passed to http.request() and it expects us to pass an object returned by the Node.js library, not the NPM package.

? I disagree... what matters is that the function returns an options object compatible with http.request(). It doesn't have to be a copy of the function used by Node.js core.

3. Maintaining an NPM module in this case is just unnecessary overhead. And you have to rely on someone else - in case they decide to drop support what are we supposed to do?

The whole function consists in like 20 line of code. If you don't want to use an external dependency you can write your own version inside your library.

4. Then let's remove the http module completely since it doesn't do anything that an ordinary Node.js code cannot also do.

By this logic Node.js core should bundle all the npm packages.

Anyway I'm not blocking this but I will also not approve it.

[szmarczak]

It doesn't have to be a copy of the function used by Node.js core.

While this is a valid point, if Node.js decides to change urlToOptions() then the NPM module will give a different result which might be incompatible. So you have to worry about backwards compatibility anyway.

The whole function consists in like 20 line of code. If you don't want to use an external dependency you can write your own version inside your library.

That's what we're doing right now. node-fetch does this too. I don't think duplicating the code is a good approach anyway.

By this logic Node.js core should bundle all the npm packages.

The opposite - expose core functions so there will be less NPM packages. See #34348

Anyway I'm not blocking this but I will also not approve it.

👍

[guybedford]

Thanks for explaining the use case here. I'm just trying to understand the exact workflows at play.

In the scenarios where you might pass the result of this call to eg http.request(), could you not just pass the URL object in as the first argument?

[szmarczak]

could you not just pass the URL object in as the first argument?

In the future, definitely :) Unfortunately we cannot do it at this point. Here's why:

1. Got caching relies on cacheable-request, which hasn't been updated yet. It expects an object.
2. It's extremely useful when normalizing the options when developing an API similar to http:

https://github.com/szmarczak/http2-wrapper/blob/bdf86601ba5853cd3ff323d5aaac8016c7db17e3/source/auto.js#L106-L109
https://github.com/szmarczak/http2-wrapper/blob/bdf86601ba5853cd3ff323d5aaac8016c7db17e3/source/client-request.js#L82-L90

Note that options override input. The end user may not pass a URL object, it may be a plain object as well.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/35454/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/35464/

[ZYSzys]

Landed in 7efada6.

[nfantone]

Sorry for intruding here - but just out of curiosity: what's urlToHttpOptions doing that you wouldn't get out of a URL already, exactly? Looks almost 1:1 to me.

> new URL('https://a:b@測試?abc#foo')
URL {
  href: 'https://a:b@xn--g6w251d/?abc#foo',
  origin: 'https://xn--g6w251d',
  protocol: 'https:',
  username: 'a',
  password: 'b',
  host: 'xn--g6w251d',
  hostname: 'xn--g6w251d',
  port: '',
  pathname: '/',
  search: '?abc',
  searchParams: URLSearchParams { 'abc' => '' },
  hash: '#foo'
}

[szmarczak]

URL does not have an auth option, port is a string (it should be a number or undefined), it doesn't have a path property. These options are used by the http module.

[nfantone]

Thanks for the clarification 👍🏼. Exactly - that's why I was saying "almost 1:1". I mean,

function urlToHttpOptions(url) {
  return {
    ...url,
    auth: `${url.username}:${url.password}`.
    port: url.port === '' ? undefined : Number(url.port),
    path: `${url.pathname ?? ''}${url.search ?? ''}`
  }
}

Not 100%, but that's pretty much it, right?

EDIT
...ok, just checked the implementation myself and I think I can answer my own question.