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

[legacy-framework] Add useParams and useRouterQuery hooks #574

Merged

Conversation

anteprimorac
Copy link
Collaborator

@anteprimorac anteprimorac commented May 28, 2020

Related: blitz-js/legacy-framework#433

What are the changes and their implications?

This PR implements two hooks useRouterParams and useRouterQuery.

Checklist

  • Tests added for changes
  • User facing changes documented

Breaking change: yes/no

No.

Other information

Example of usage:

// app/locations/pages/locations/[postalCode].tsx
import { useRouterParams, useRouterQuery, useQuery } from 'blitz';

export const Locations = () => {
  const { postalCode } = useRouterParams();
  const { order, orderby } = useRouterQuery();
  const [locations] = useQuery(getLocations, { where: { postalCode }, orderBy: { [orderby]: order } });
  return locations;
};

Copy link
Member

@flybayer flybayer left a comment

Choose a reason for hiding this comment

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

Ah nice, very clever!

I think useRouterParams can simply rely on router.query and remove any keys found in useRouterQuery unless the value is different.

So for route /[id] with pathname /1?id=2&name=fox:

router.query useRouterParams useRouterQuery
id = 1 id = 1 id = 2
name = fox name = fox

Can you also add some tests for this?

Lastly, before merging this we should resolve our discussion in #530 about number parsing. Maybe we can have useParamsStr, useParamsInt, and useRouterQuery

packages/core/src/use-router-query.ts Show resolved Hide resolved
@anteprimorac anteprimorac requested a review from flybayer May 28, 2020 20:34
@flybayer
Copy link
Member

@anteprimorac can you also add at least one unit test for useRouterParams?

@Zeko369
Copy link
Collaborator

Zeko369 commented May 29, 2020

@anteprimorac Also rerun prettier with our config, it seems like some files weren't formatted correctly

Zeko369
Zeko369 previously approved these changes May 29, 2020
@anteprimorac
Copy link
Collaborator Author

@anteprimorac can you also add at least one unit test for useRouterParams?

I've changed store example to use useRouterParams here ba730b1#diff-1b842ad42068e396b7c21d9376384d87R8 and integration test is run over it. Is that enough?

It's hard to write unit tests because it depends on useRouter hook from next.js. Not sure if they provide a mock for it.

@anteprimorac Also rerun prettier with our config, it seems like some files weren't formatted correctly

🤔 it looks like there is a separate prettier configuration for each example. Going to fix it now. Thanks for pointing this.

@anteprimorac anteprimorac requested a review from flybayer May 29, 2020 20:59
Copy link
Member

@flybayer flybayer left a comment

Choose a reason for hiding this comment

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

Ok, great! Few more minor things :)

packages/core/package.json Outdated Show resolved Hide resolved
packages/core/src/use-router-params.ts Outdated Show resolved Hide resolved
packages/core/test/use-router-params.test.ts Outdated Show resolved Hide resolved
README.md Outdated
@@ -270,6 +270,7 @@ Thanks to these wonderful people ([emoji key](https://allcontributors.org/docs/e

<!-- markdownlint-enable -->
<!-- prettier-ignore-end -->

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@merelinguist this change was introduced by husky pre-commit hook

@anteprimorac anteprimorac requested a review from flybayer May 30, 2020 15:15
@anteprimorac
Copy link
Collaborator Author

@flybayer if you agree, we can merge this as it is and gather more info on what to provide regarding validation, sanitization and casting of params and queries.

@flybayer
Copy link
Member

flybayer commented Jun 2, 2020

@anteprimorac let's wait until we resolve our discussion in blitz-js/legacy-framework#433. I just made another comment.

And if we add useParamNumber() and friends, I think it'd be better to use useParams() instead of useRouterParams() so all the param hooks are the same, and it's only query that is different. Additionally, all (or most) of other routers have useParams() so this will be more natural/intuitive for many folks.

@flybayer
Copy link
Member

flybayer commented Jun 3, 2020

@anteprimorac what do you think about also changing useRouter() to have params and query to align with these new hooks?

Current:

// Everything lumped in query
const {query} = useRouter()

Idea:

// Separate route params from query string items
const {params, query} = useRouter()

This would be a breaking change for existing blitz apps

@anteprimorac
Copy link
Collaborator Author

@flybayer that’s an excellent idea. We could provide useRouter as a wrapper around the hook from next.js

@flybayer
Copy link
Member

flybayer commented Jun 3, 2020

Yes exactly. I think we should do that

@flybayer flybayer changed the title Add useRouterParams and useRouterQuery hooks Add useParams and useRouterQuery hooks Jun 4, 2020
@flybayer flybayer merged commit 280755c into blitz-js:canary Jun 4, 2020
@itsdillon itsdillon changed the title Add useParams and useRouterQuery hooks [legacy-framework] Add useParams and useRouterQuery hooks Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants