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 a test for stringified searchParams in merge #1208

Merged
merged 9 commits into from May 10, 2020

Conversation

jaulz
Copy link
Contributor

@jaulz jaulz commented Apr 30, 2020

While using the pagination API I see that the searchParams option is a string and thus it fails at this line.

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates.

@jaulz jaulz changed the title fix: catch case where searchParams is a string [WIP] fix: catch case where searchParams is a string Apr 30, 2020
@jaulz jaulz changed the title [WIP] fix: catch case where searchParams is a string fix: catch case where searchParams is a string Apr 30, 2020
@jaulz jaulz changed the title fix: catch case where searchParams is a string fix: handle stringified searchParams in merge Apr 30, 2020
@sindresorhus sindresorhus changed the title fix: handle stringified searchParams in merge Handle stringified searchParams in merge May 3, 2020
@szmarczak
Copy link
Collaborator

Fixed in 157e02b

@szmarczak szmarczak closed this May 3, 2020
@jaulz
Copy link
Contributor Author

jaulz commented May 3, 2020

Great, thanks!

@szmarczak
Copy link
Collaborator

szmarczak commented May 3, 2020

Actually I'm wrong. e23b7f9 Sorry 😅

@szmarczak szmarczak reopened this May 3, 2020
@szmarczak
Copy link
Collaborator

You just need to revert your commit and git pull origin master --rebase. Then you can start working on the fix :)

@jaulz
Copy link
Contributor Author

jaulz commented May 4, 2020

@szmarczak I just pushed another change which 1) handles searchParams that are passed string, undefined or instances of URLSearchParams via defaults and 2) prevents duplications of search params. Though, I'm not sure if the second will cause regressions because it changes the behaviour how searchParams are merged. However, in the context of paging there would be no other way to force setting only one key in the URL as it would result in a URL like this: ?page=1&page=2&page=3&page=4... (not sure if you meant that with your previous comment).

source/core/index.ts Outdated Show resolved Hide resolved
@szmarczak
Copy link
Collaborator

szmarczak commented May 4, 2020

I don't think you fixed 1) because as mentioned before it worked even without your changes. You need to provide another test if you think otherwise ;) sorry I was wrong again

jaulz and others added 2 commits May 4, 2020 13:32
Co-authored-by: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>
@szmarczak szmarczak changed the title Handle stringified searchParams in merge Add a test for stringified searchParams in merge May 10, 2020
@szmarczak szmarczak merged commit 7d7361c into sindresorhus:master May 10, 2020
@szmarczak
Copy link
Collaborator

This PR partially is covered by #1229 Fixed the merge conflict. Thanks! 🙌

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