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 support for search prop #120

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

jmisasa
Copy link
Contributor

@jmisasa jmisasa commented Mar 23, 2024

This PR adds support for search prop which while using the library we noticed it was yet not supported

Thanks a lot for this amazing package!

Copy link

changeset-bot bot commented Mar 23, 2024

🦋 Changeset detected

Latest commit: 5afd47b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
next-router-mock Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jmisasa
Copy link
Contributor Author

jmisasa commented Mar 23, 2024

I noticed this PR #82 where node dependencies were eliminated. I just pushed some new changes that should address this.

Didn't go with straight URLSearchParams because of the "bracket" notation required to parse a URL that contains multiple values for the same key

With query-string this behaviour is allowed:

import queryString from 'query-string';

queryString.parse('foo=1&foo=2&foo=3');
//=> {foo: ['1', '2', '3']}

Happy to read your thoughts. Thank you!


EDIT: ah pushed commit is messed up because it still uses node querystring and not query-string! when switching there seems to be some issue with types. Marked as draft and will have a clearer look at this after a good night sleep

@jmisasa jmisasa marked this pull request as draft March 23, 2024 04:12
@jmisasa jmisasa marked this pull request as ready for review March 23, 2024 10:58
@jmisasa
Copy link
Contributor Author

jmisasa commented Mar 23, 2024

ended up removing external dependency and used parsedUrl directly, feels kind of hacky but seems to do the trick. Thanks again!

Copy link
Owner

@scottrippey scottrippey left a comment

Choose a reason for hiding this comment

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

I love the tests, great job! Thanks for reusing the existing parsing logic, rather than adding dependencies.
Some tiny comments below:

src/urls.ts Outdated Show resolved Hide resolved
src/MemoryRouter.test.tsx Show resolved Hide resolved
src/MemoryRouter.tsx Outdated Show resolved Hide resolved
@scottrippey scottrippey merged commit b0eed5a into scottrippey:main Mar 27, 2024
4 checks passed
@jmisasa jmisasa deleted the add-support-for-search-prop branch April 30, 2024 19:43
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