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

Fix removeSingleSlash option adding slashes #122

Merged
merged 1 commit into from Sep 30, 2020

Conversation

silverwind
Copy link
Contributor

Turns out the option caused single slashes to be added to URLs that had none because the URL constructor defaults pathname to / both on parse and .toString(), so I had to prevent overwriting urlString to be able to check if the passed URL's pathname is /.

Also added an entry for the option to index.test-d.ts.

Turns out the option caused single slashes to be added to URLs that had
none because the `URL` constructor defaults pathname to `/` both on parse
and .toString(), so I had to prevent overwriting `urlString` to be able
to check if the passed URL's pathname is `/`.

Also added an entry for the option to `index.test-d.ts`.
@sindresorhus
Copy link
Owner

Turns out the option caused single slashes to be added to URLs that had none because the URL constructor defaults pathname to / both on parse and .toString(), so I had to prevent overwriting urlString to be able to check if the passed URL's pathname is /.

Can you add a regression test for that?

@silverwind
Copy link
Contributor Author

Two of the added test cases already cover this, they will fail on master:

https://github.com/sindresorhus/normalize-url/pull/122/files#diff-1dd241c4cd3fd1dd89c570cee98b79ddR138
https://github.com/sindresorhus/normalize-url/pull/122/files#diff-1dd241c4cd3fd1dd89c570cee98b79ddR148

It's not strictly a regression, more like an improvement for when removeSingleSlash is set to false.

@sindresorhus sindresorhus changed the title Fix removeSingleSlash option adding slashes Fix removeSingleSlash option adding slashes Sep 30, 2020
@sindresorhus sindresorhus merged commit 1e06753 into sindresorhus:master Sep 30, 2020
@silverwind silverwind deleted the fixslash branch September 30, 2020 10:58
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 this pull request may close these issues.

None yet

2 participants