Skip to content
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

Expose the url-to-options function from internal/url.js #34349

Closed
szmarczak opened this issue Jul 14, 2020 · 5 comments
Closed

Expose the url-to-options function from internal/url.js #34349

szmarczak opened this issue Jul 14, 2020 · 5 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@szmarczak
Copy link
Member

Is your feature request related to a problem? Please describe.

#14570 (comment)

Describe the solution you'd like

Expose the urlToOptions module so it can be imported e.g. const {urlToOptions} = require('url');

That way we could easily convert a URL instance without duplicating the code like this.

Describe alternatives you've considered

No, duplicates increase the package size.

@bnoordhuis bnoordhuis added the feature request Issues that request new features to be added to Node.js. label Jul 14, 2020
@bnoordhuis
Copy link
Member

I'm inclined to say it's better to copy it out into a npm module than to expose it because:

  1. it's borderline trivial

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

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

@szmarczak
Copy link
Member Author

it's borderline trivial

I strongly disagree. If this was trivial then it wouldn't be used in the http module to deserialize any URL instance.

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

Hmm... After giving it some thought I'd go for const {urlToOptions} = require('http'); since it returns HTTP options.

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

That's the cost you always have to take. It's already used in the http module so you already need to worry about this.

@bnoordhuis
Copy link
Member

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

It it needed runtime magic or if you needed to go to extreme lengths to make it do something that's trivial for core code, that's a compelling argument to expose it, but that doesn't apply here.

There are exceptions to the rule (tls.checkServerIdentity() is one) but the threshold is pretty high.

It's already used in the http module so you already need to worry about this.

The distinction here is that it's only indirectly observable to user code. For example, we don't have to worry about callers passing something that's not a URL object.

@szmarczak
Copy link
Member Author

szmarczak commented Jul 14, 2020

It it needed runtime magic or if you needed to go to extreme lengths to make it do something that's trivial for core code, that's a compelling argument to expose it, but that doesn't apply here.

Then let's completely remove the http (client) module since we can achieve the same using undici.

You get me wrong, I just don't want to duplicate what already exists in the Node.js core.

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.

@ZYSzys
Copy link
Member

ZYSzys commented Nov 4, 2020

#35960

@ZYSzys ZYSzys closed this as completed in 7efada6 Jan 17, 2021
ruyadorno pushed a commit that referenced this issue Jan 22, 2021
PR-URL: #35960
Fixes: #34349
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this issue Aug 8, 2021
PR-URL: #35960
Fixes: #34349
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs pushed a commit that referenced this issue Aug 12, 2021
PR-URL: #35960
Fixes: #34349
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 31, 2021
PR-URL: #35960
Fixes: #34349
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
foxxyz pushed a commit to foxxyz/node that referenced this issue Oct 18, 2021
PR-URL: nodejs#35960
Fixes: nodejs#34349
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants