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

feat(router): add support for search params #5094

Merged
merged 4 commits into from
Nov 8, 2023
Merged

feat(router): add support for search params #5094

merged 4 commits into from
Nov 8, 2023

Conversation

bjoerge
Copy link
Member

@bjoerge bjoerge commented Oct 31, 2023

Description

This PR adds support for a special key on the url state: _searchParams , which is an array of [key: string, value: string] tuples (similar to what you'd get from URLSearchParams.entries()).

It uses the scope (aka namespace) feature of the router, so when you're within a scope you don't need to worry about how your own search params fits in with the global environment. In the url, they will be namespaced with /some/route?scope[param]=value. Params in nested scopes will be scopeA[scopeB][param]=value (see added unit tests for details).

Usage example

Given the route /path/to/:someParam

Read search params

Given the url: /path/to/something?foo=bar&bar=baz

const router = useRouter()

console.log(router.state)

Will log

{
  someParam: 'something',
  _searchParams: [['foo', 'bar'], ['bar', 'baz']]
//^ see note about prefix below 
}

Navigate w/search params

const router = useRouter()

router.navigate({
  someParam: 'something',
  _searchParams: [['foo', 'bar'], ['bar', 'baz']]
})

Will navigate to:

/path/to/something?foo=bar&bar=baz

What to review

  • Does the API make sense?
  • I chose the _-prefix, which isn't strictly speaking safe as someone could potentially (although quite unlikely) have an existing route with _searchParams as a param (e.g. /path/to/:_searchParams), which will then be in conflict. If we want to play it extra safe, we can consider changing to $searchParams. We currently don't allow using $ in url params (i.e. defining a route as /path/to/:$param throws an error).

Note: this PR also recovers the test suite and readme that somehow didn't make it when we moved this from @sanity/state-router over to the sanity-package

Notes for release

  • Adds support for search params in sanity/router

@vercel
Copy link

vercel bot commented Oct 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Nov 3, 2023 0:16am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 3, 2023 0:16am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Nov 3, 2023 0:16am

@github-actions
Copy link
Contributor

No changes to documentation

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2023

Component Testing Report Updated Nov 3, 2023 12:17 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 12s 12 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 3s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 8s 3 0 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 6s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 17s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 8s 9 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 38s 18 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 6s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 4s 3 0 0

Copy link
Member

@mariuslundgard mariuslundgard left a comment

Choose a reason for hiding this comment

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

I'm worrying that the API is unconventional. Did you consider this approach?

Given the url: /path/to/something?foo=bar&bar=baz

const router = useRouter()

console.log(router.state, router.searchParams)

Will log

{someParam: 'something'}, {foo: 'bar', bar: 'baz'}

Navigate w/search params

router.navigate({
  someParam: 'something',
}, {
  searchParams: {foo: 'bar', bar: 'baz'}
})

@bjoerge
Copy link
Member Author

bjoerge commented Nov 7, 2023

Can you elaborate on what you mean by unconventional, and what problems you think it may cause?

I did consider it! But the url params is part of the state, so it felt natural to keep it there. It will make it easier to pass it around as a unified object representing the full picture of the current extracted url state.

This, in addition to the fact that it would be harder to implement in a backwards compatible way (router.encode/decode is part of our public API), made me go for this solution, but open to reconsider if you can help me understand what real problems it can cause.

If we didn't have to care about backwards compat, I think a better option might be if the state object itself had a distinction between url params and search params, e.g. using the example above:

Given the url: /path/to/something?foo=bar&bar=baz

const router = useRouter()

console.log(router.state)

Will log (something like)

{
  pathParams: {
    someParam: 'something'
  },
  searchParams: [['foo', 'bar'], ['bar', 'baz']]
}

Noting this as a potential future improvement

@mariuslundgard
Copy link
Member

mariuslundgard commented Nov 7, 2023

@bjoerge This part looks especially unconventional/foreign:

_searchParams: [['foo', 'bar'], ['bar', 'baz']]

Would be more readable like this:

_searchParams: {foo: 'bar', bar: 'baz'}

And ideally live outside of the state object so it doesn't need the _ or $ prefix at all.

@bjoerge
Copy link
Member Author

bjoerge commented Nov 8, 2023

_searchParams: {foo: 'bar', bar: 'baz'}

It's completely valid to have multiple search params with the same name (which is why URLSearchParams isn't a "map-like" structure, but has, for example, a getAll('<param>') => string[] method.

If we made it a record/map-like it would have to be:
_searchParams: {foo: ['bar'], bar: ['baz']}

But as noted in the PR description, this format is aligned with what you get from URLSearchParams().entries(), (and also other .entries() apis, etc.), so I it's a pretty common way of representing key/value pairs. You can also pass it as-is to the URLSearchParams constructor, e.g. new URLSearchParams([['foo', 'bar'], ['bar', 'baz']]).

Given that this PR is approved, I'll merge it for now. It should be trivial to convert the entries array into something more convenient, e.g. by using Object.fromEntries(searchParams) (if you know you're not dealing with multiple params) or
new URLSearchParams(searchParams) if you don't.

@bjoerge bjoerge added this pull request to the merge queue Nov 8, 2023
Merged via the queue into next with commit 50ac0b1 Nov 8, 2023
34 checks passed
@bjoerge bjoerge deleted the sdx-722 branch November 8, 2023 10:48
bjoerge added a commit that referenced this pull request Nov 8, 2023
* chore(router): restore pre-v3 docs and test suite

* test(router): add (failing) tests for urlSearchParams

* feat(router): add support for search params

Unit test changes:
- improve error messages
- add a few more test cases
- update findMatchingNode tests with new internal format

* fix(core): include search params when parsing url in workspace router
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.

3 participants