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

Add removePort option #171

Closed
ianizaguirre opened this issue Jul 20, 2022 · 7 comments · Fixed by #174
Closed

Add removePort option #171

ianizaguirre opened this issue Jul 20, 2022 · 7 comments · Fixed by #174

Comments

@ianizaguirre
Copy link
Contributor

ianizaguirre commented Jul 20, 2022

In the Docs, you have an example that has a port on a url and then when you wrap the url with "normalizeUrl" the port is no longer included:
image

When I run this same example locally, the Port :80/ is still included - which is different behavior from what your documentation example above shows:
image

How would I remove the :80/ port portion of a url with your package ?

I am pretty sure there is a bug because when I did another test the behavior is not consistent. As you can see in the image below, the results are not consistent just by changing a url from having http too https and also I see that just changing the actual number for the port in the url makes the behavior different too:
image

@ianizaguirre ianizaguirre changed the title How do you remove the port from a url? Bug Found when removing the port from a url? Jul 20, 2022
@sindresorhus
Copy link
Owner

sindresorhus commented Jul 20, 2022

Why would it remove 80? The default port for HTTPS is 443. I think the existing behavior is correct.

@ianizaguirre
Copy link
Contributor Author

ianizaguirre commented Jul 21, 2022

Why would it remove 80? The default port for HTTPS is 443. I think the existing behavior is correct.

Hi, Thanks for your quick reply.

hmm, well in my case I wanted an option to remove the port so I can create a more user friendly/readable url. I used your library but had to do an additional step of regex to strip the :# port number from it.

I had assumed your library would have let me remove the port (any port) on the url, instead of it deciding when a port should be removed or kept.

Do you think that adding an option to remove a port (any port found) to your library would be a feature you could implement?
It could be added with the ability to pass in true to remove all ports or it can also accept regex, like how you set it up for "removeDirectoryIndex":

normalizeUrl('www.sindresorhus.com/foo/default.php', {
	removeDirectoryIndex: [/^default\.[a-z]+$/]
});

it could be named something like "removePort"

@sindresorhus
Copy link
Owner

Removing port 80 from a HTTPS URL would could potentially break the URL, although unlikely. I'm ok with a removePort: true option. I think a boolean is enough for now. If people have a need for more, they can request it.

@sindresorhus sindresorhus changed the title Bug Found when removing the port from a url? Add removePort option Sep 1, 2022
@ianizaguirre
Copy link
Contributor Author

ianizaguirre commented Sep 1, 2022

Nice, I can work on this task.

Just to get some clarity before I make a pull request, when you said:

Removing port 80 from a HTTPS URL would could potentially break the URL, although unlikely.

When I add the removePort boolean option, I still make it remove all ports including port 80, right - since like you mentioned its unlikely to cause an issue?

@loynoir
Copy link

loynoir commented Sep 1, 2022

@ianizaguirre

I think removePort should be rename to unsafeRemovePort, or current naming style forceRemovePort.

Because https://example.com:80 and https://example.com is NOT SAME AT ALL, they MAY just happen to LOOK same in specific cases.

This suggestion may prevent confusion with the safe remove port, like remove :80 from http and remove :443 from https.

@ianizaguirre
Copy link
Contributor Author

ianizaguirre commented Sep 2, 2022

ok sounds good. I will work on making a pull request for this

@ianizaguirre
Copy link
Contributor Author

ianizaguirre commented Sep 6, 2022

@sindresorhus

Pull request has been created: #174

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants