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

Accept a boolean for the removeQueryParameters option #136

Merged
merged 5 commits into from
Jun 22, 2021

Conversation

SimonJang
Copy link
Contributor

@SimonJang SimonJang commented Jun 19, 2021

I saw issue #133 as unresolved so I went ahead and gave it a go.

  • Add support for removeQueryParameters boolean flag and implemented the new behavior
  • Updated the documentation and TS types
  • Added a couple of new tests.

Any feedback is welcome.

Fixes #133

index.js Outdated
@@ -187,6 +187,10 @@ const normalizeUrl = (urlString, options) => {
// Take advantage of many of the Node `url` normalizations
urlString = urlObj.toString();

if (options.removeQueryParameters === true) {
urlString = urlString.split('?')[0];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not safe. Instead, modify the URL object.

removeQueryParameters: false
});
//=> 'http://www.sindresorhus.com/?foo=bar&ref=test_ref&utm_medium=test'
```
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index.d.ts and readme should be fully in sync.

readme.md Outdated
//=> 'http://sindresorhus.com'
```

`false` will not remove any query parameter. Other options like `sortQueryParameters` are still applied when toggled.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I see the point of:

Other options like sortQueryParameters are still applied when toggled.

That should already be obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it.

@sindresorhus sindresorhus changed the title Add removeQueryParameters boolean option - #133 Accept a boolean for the removeQueryParameters option Jun 19, 2021
@SimonJang
Copy link
Contributor Author

SimonJang commented Jun 20, 2021

Thanks for the feedback. PR has been updated!

index.js Outdated
@@ -173,6 +173,12 @@ const normalizeUrl = (urlString, options) => {
}
}

if (options.removeQueryParameters === true) {
for (const key of [...urlObj.searchParams.keys()]) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just set urlObj.search = '';.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I didn't knew that. Changed it.

@sindresorhus
Copy link
Owner

Thanks for the feedback. PR has been updated!

Everything is not resolved.

@SimonJang
Copy link
Contributor Author

Thanks for the feedback. PR has been updated!

Everything is not resolved.

Should all be resolved now.

@sindresorhus
Copy link
Owner

You still need to do https://github.com/sindresorhus/normalize-url/pull/136/files#r654795467

@SimonJang
Copy link
Contributor Author

You still need to do #136 (files)

Should be fully in sync now.

@sindresorhus sindresorhus merged commit 6216336 into sindresorhus:main Jun 22, 2021
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.

Strip query parameters / search
2 participants