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

Option to keep trailing slash in all cases #119

Closed
silverwind opened this issue Sep 28, 2020 · 7 comments · Fixed by #120
Closed

Option to keep trailing slash in all cases #119

silverwind opened this issue Sep 28, 2020 · 7 comments · Fixed by #120

Comments

@silverwind
Copy link
Contributor

silverwind commented Sep 28, 2020

I'd like to keep the output of this module as close as possible to the input and that means to also retain a pathname of / in a case like this:

normalizeUrl('http://sindresorhus.com/', {removeTrailingSlash: false});
//=> 'http://sindresorhus.com'

While I generally think removeTrailingSlash: false should not remove any slashes in pathname, I could see a option like keepTrailingSlashOnEmptyPath being added for compatibilty.

This option would be useful because it is hard to work around because URL() will always output a pathname of / regardless whether that slash is present or not:

> new URL("https://sindresorhus.com").pathname
'/'
> new URL("https://sindresorhus.com/").pathname
'/'
@sindresorhus
Copy link
Owner

I'd like to keep the output of this module as close as possible to the input

Why does it matter? There are many things this package does that change the input. That's the whole point of it. What's your use-case for needing this?

I could see a option like keepTrailingSlashOnEmptyPath being added for compatibilty.

Yes, it would have to be a separate option.

@silverwind
Copy link
Contributor Author

silverwind commented Sep 28, 2020

There are many things this package does that change the input

Yeah and I have those all turned off for my case. But this trailing slash stripping is the only one I can not turn off.

What's your use-case for needing this?

I'm using this module to URI-encode URL segments, punycode IDNs and get rid of double-URL-encoding which are all valuable adjustments in itself.

I think stripping the trailing slash is a opinionated change that even goes against WHATWG recommendations to always have pathname set to / when not present. Browsers also add this slash after entering a URL without one.

Ultimately, my URLs are going to end up in a HTTP Location header and I know from experience that some web servers may not behave as expected when pathname is empty (e.g they will see a GET command, unless the client that follows the location corrects this).

@silverwind
Copy link
Contributor Author

silverwind commented Sep 28, 2020

Maybe we could add a third value "none" to the removeTrailingSlash option for this.

@sindresorhus
Copy link
Owner

Alright. I'm open to adding something for this.

@sindresorhus
Copy link
Owner

Maybe we could add a third value "none" to the removeTrailingSlash option for this.

I don't think that would be clear enough. none is kinda like false in my mind.

@silverwind
Copy link
Contributor Author

silverwind commented Sep 28, 2020

Any better ideas for overloading removeTrailingSlash or would it better be a new option? Maybe keepSlashPath for a new option name but I'd prefer to overload removeTrailingSlash.

silverwind added a commit to silverwind/normalize-url that referenced this issue Sep 28, 2020
Decided it's better to have a separate option than to overload a boolean
type with non-boolean values.

Fixes: sindresorhus#119
@silverwind
Copy link
Contributor Author

#120

silverwind added a commit to silverwind/normalize-url that referenced this issue Sep 28, 2020
Decided it's better to have a separate option than to overload a boolean
type with non-boolean values.

Fixes: sindresorhus#119
silverwind added a commit to silverwind/normalize-url that referenced this issue Sep 28, 2020
Decided it's better to have a separate option than to overload a boolean
type with non-boolean values.

Fixes: sindresorhus#119
silverwind added a commit to silverwind/normalize-url that referenced this issue Sep 28, 2020
Decided it's better to have a separate option than to overload a boolean
type with non-boolean values.

Fixes: sindresorhus#119
silverwind added a commit to silverwind/normalize-url that referenced this issue Sep 28, 2020
Decided it's better to have a separate option than to overload a boolean
type with non-boolean values.

Fixes: sindresorhus#119
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants