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

url.format does not have same behavior for pathnames with and without slashes #19523

Closed
beck opened this issue Mar 21, 2018 · 6 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. url Issues and PRs related to the legacy built-in url module. wontfix Issues that will not be fixed.

Comments

@beck
Copy link

beck commented Mar 21, 2018

Using Node@9.9 on OSX.

Problem

url.format returns an usable result when pathname is not slash prefixed:

> const url = require('url')
> url.format({hostname: 'dougbeck.me', pathname: 'womp.html'})
'dougbeck.mewomp.html'

Expectation

pathname is treated the same with or without the leading / (slash)

Conversation started here: nodejs/help#1176

@addaleax addaleax added confirmed-bug Issues with confirmed bugs. url Issues and PRs related to the legacy built-in url module. labels Mar 21, 2018
@mithunsasidharan
Copy link
Contributor

mithunsasidharan commented Mar 23, 2018

@beck From what I understand looking at the code, adding missing FORWARD_SLASH logic only applies to slashedProtocol like http, https and so on. So if we pass the protocol to format like below it handles it well :

> const url = require('url')
> url.format({protocol: 'http', hostname: 'dougbeck.com', pathname: 'womp.html'})
'http://dougbeck.com/womp.html'

One solution is to take out the logic to add missing FORWARD_SLASH from the slashedProtocol condition and make it generic. I'd like to get more inputs before going ahead with the PR. @addaleax

@apapirovski
Copy link
Member

I'm not sure about this one... There are protocols that don't have slashes before pathname (e.g. mailto). I suppose we could check that hostname & pathname exist but given that the legacy url stuff is meant to be frozen, I don't know how I feel about it.

@beck
Copy link
Author

beck commented Apr 11, 2018

the legacy url stuff is meant to be frozen

👍I'm cool with this being labeled confirmed and wontfix.

If anyone else stumbles upon this, this is the helper I used to give me the API I desired.

@mithunsasidharan
Copy link
Contributor

@apapirovski @beck : Passing the protocol along is probably the safe thing to do. Given its the legacy stuff, I agree that probably it can be left as is.

@lpinca
Copy link
Member

lpinca commented Apr 15, 2018

Another option is to use slashes:

url.format({ slashes: true, hostname: 'dougbeck.com', pathname: 'womp.html' })

or url.URL as already suggested in nodejs/help#1176.

@lpinca lpinca added the wontfix Issues that will not be fixed. label Apr 15, 2018
@lpinca
Copy link
Member

lpinca commented Apr 15, 2018

Closing, let me know if it should stay open.

@lpinca lpinca closed this as completed Apr 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. url Issues and PRs related to the legacy built-in url module. wontfix Issues that will not be fixed.
Projects
None yet
Development

No branches or pull requests

5 participants