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

Custom useParams hook #433

Closed
Zeko369 opened this issue May 20, 2020 · 33 comments · Fixed by blitz-js/blitz#637
Closed

Custom useParams hook #433

Zeko369 opened this issue May 20, 2020 · 33 comments · Fixed by blitz-js/blitz#637

Comments

@Zeko369
Copy link
Contributor

Zeko369 commented May 20, 2020

What do you want and why?

As mentioned in the slack, current way of getting route params isn't really as nice as it could be

const router = useRouter()
const id = parseInt(router?.query.id as string)

Better API (similar to prisma client)

// returns ID as number
const {id} = useParams({id: true}); // Or maybe have 'number'?
// returns ID as string
const {id} = useParams({id: 'string'});
// Custom parser for hex strings
const {id} = useParams({id: (raw: string) => parseInt(raw, 16)});

// Multiple params
const {projectId, slug} = useParams({projectId: true, slug: 'string'});

// Maybe have a shorthand syntax with an array
const {id} = useParams(['id']); // id is number by default

Also if we have an object we can go overboard with TS definitions (keyof stuff) and have a lot nicer API for getting this stuff

I'd like to build this, but before I'd like to discuss maybe there's some other ideas on how to improve this

Insipiration

In a CRA project of mine

// useParamsInt.ts
import { useParams } from 'react-router-dom';

const useParamsInt = (key = 'id') => {
  const params = useParams();
  return params[key] ? parseInt(params[key]) : null;
};

export default useParamsInt;

// component.tsx
const id = useParamsInt();
const chapter_id = useParamsInt('chapter_id');
const page_id = useParamsInt('page_id');
@flybayer
Copy link
Member

I totally agree we need this. I was just thinking about this the other day.

The whole number/string thing is a real downer lol.

I think the majority of folks will use all number ids or all string ids. So I think it should be super simple for either of these.

How about this?

// Always returns numbers - no option argument
const {taskId} = useParamsInt()

// Always returns strings - no option argument
const {taskId} = useParamsStr()

// Escape hatch for a mixed bag
const {taskId} = useParamsInt()
const router = useRouter()
const projectId = router?.query.projectId as string

@Zeko369
Copy link
Contributor Author

Zeko369 commented May 21, 2020

@flybayer I think having more powerful useParamsCustom() which would take a prisma query like object (key -> boolean | function, where a function would be a parser function and true would just parseInt) would be quite useful, just to make it a bit more readable for some stuff.

Also I'd change your example

// Always returns strings - no option argument
const {id} = useParamsStr()

// Always returns numbers - no option argument
const {id} = useParamsInt()

// Always returns numbers - no option argument
const {taskId} = useParamsInt('taskId')

// Multiple in one
const {id, taskId} = useParamsInt(['id', 'taskId'])

Also add the custom hook back with a bit different syntax

// Custom parsers
const {id, taskId} = useParamsCustom({
    id: true,
	taskId: (raw) => parseInt(raw.split('-')[0], 16)
});

// Impl without last hook
const {id} = useParamsInt();
const {taskId as taskIdRaw} = useParamsStr('taskId');
const taskId = parseInt(taskIdRaw.split('-')[0], 16);

// Or
const {id} = useParamsInt();
const router = useRouter();
const taskIdRaw = router?.query.taskId as string;
const taskId = parseInt(taskIdRaw.split('-')[0], 16)

@flybayer
Copy link
Member

@Zeko369

What would this do useParamsInt('taskId') vs useParamsInt()?

Adding useParamsCustom could help some people, but I think we should wait until we actually having people asking for it. We have to be conservative in adding new apis because this will only grow over time. We want to make sure we only add things people will use.

Also, anyone can create their own custom hooks like this and publish to npm, so something like this is different that something core in the CLI or server logic.

@Zeko369
Copy link
Contributor Author

Zeko369 commented May 22, 2020

By default we can't know what routing name the user chose

  • [id].tsx => we can match by default
  • [fooBar].tsx => we can't match by default (need to add 'fooBar' as param)
const {id} = useParamsInt();
const {fooBar} = useParamsInt('fooBar');
  • [projectId]
    • [id].tsx => we can't know what you want
    • ...
const {projectId, id} = useParamsInt(['projectId', 'id']);

@flybayer
Copy link
Member

Oh, so I was thinking useParamsInt() will return all query items. We don't have to know what the name is. Basically just return router.query. Right?

@MrLeebo
Copy link
Member

MrLeebo commented May 23, 2020

What would you do with invalid values that end up in the param? I don't think NaN is a contextually appropriate value to return for a URL param. Plus parseInt() and Number() both have edge cases and special behaviors when it comes to values that they'll parse which you may not want (e.g. hex values, leading zeroes in older browsers, strings that start with numbers get truncated on the first non-digit, loss of precision for numbers with a lot of significant digits, exponents, etc)

Most apps may want to just error out early if the route param is fails to cast to integer, but others may prefer to try to fix the value or do something custom with it. I don't think a hook we provide should ever encourage the app to try to do a lookup from a value that got truncated. Seems difficult for a single hook to try to satisfy all of those possible scenarios without adding clunkiness.

@Zeko369
Copy link
Contributor Author

Zeko369 commented May 23, 2020

Oh, so I was thinking useParamsInt() will return all query items. We don't have to know what the name is. Basically just return router.query. Right?

We could just do that, seems easier than my implementation 😅

Most apps may want to just error out early if the route param is fails to cast to integer, but others may prefer to try to fix the value or do something custom with it. I don't think a hook we provide should ever encourage the app to try to do a lookup from a value that got truncated. Seems difficult for a single hook to try to satisfy all of those possible scenarios without adding clunkiness.

We could just return null if we get a NaN

@Zeko369 Zeko369 self-assigned this May 23, 2020
@MrLeebo
Copy link
Member

MrLeebo commented May 25, 2020

Returning null instead of throwing an error because the value isn't what you expected is Error Swallowing. If throwing an error is out of the question then I think at least rename the hook to make it apparent it is swallowing e.g. useSafeParamsInt()

@flybayer
Copy link
Member

Excellent points @MrLeebo

Maybe the best way to "solve" this is switch ids to be strings by default, which I think we should do anyways.

So then the remaining issue is that router.query keys can be string or string[]. It would be string[] if the path is like /blog/[...slug].

So maybe we could add useIdParams() that only returns the router.query keys that are string | undefined. Arrays would be excluded.

@Zeko369
Copy link
Contributor Author

Zeko369 commented May 26, 2020

I still think that most people would rather go with integer IDs for starters

@anteprimorac
Copy link
Contributor

anteprimorac commented May 26, 2020

Not sure if this is inherited from next.js router, but I think params /projects/[id] and query /projects/?order=asc should be in separate objects router.params and router.query.

This might be an edge case, but what happens when param and query share the same name, e.g. /projects/[id]?id=2?

Most apps may want to just error out early if the route param is fails to cast to integer, but others may prefer to try to fix the value or do something custom with it. I don't think a hook we provide should ever encourage the app to try to do a lookup from a value that got truncated. Seems difficult for a single hook to try to satisfy all of those possible scenarios without adding clunkiness.

@MrLeebo I agree with you. URL params and query are basically user input and if we are using that input for the database query then it needs to be validated and sanitized.

@flybayer
Copy link
Member

flybayer commented May 27, 2020

How about useParams() automatically convert number strings to numbers and return the rest as strings?

I think this is the output you intuitively expect.

// router.query = {id: "123", somethingElse: "word"}

const params = useParams()

// params = {id: 123, somethingElse: "word"}

We can do a regex test, and if it only has digits, attempt parseInt. Falling back to string if it fails. Floats would not be parsed and passed through as a string. And do the same for any other edge cases that exist with parseInt

@anteprimorac
Copy link
Contributor

Not every number looking string should be converted to a number.

For the first iteration, we could just return a string dictionary { [key: string]: string } containing all params and let the developer decide how to deal with it.

@flybayer
Copy link
Member

@anteprimorac I'm curious, when would you want a number to remain a string in this context?

@anteprimorac
Copy link
Contributor

anteprimorac commented May 27, 2020

@flybayer For example if you have a list of locations and you want to filter them by postal code and you want to create a nice looking route like this /locations/[postalCode]. Postal code in many cases could be safely converted to a number but there are cases where it should not, for example, a valid postal code in the US is 00501 which converted to a number is 501.

@flybayer
Copy link
Member

Yeah that's a great example!

What I would like the most is if Prisma would allow both string & number inputs (like Rails).

But until then, I think our current best option is probably this:

// Always returns numbers
const {taskId} = useParamsInt()

// Always returns strings
const {taskId} = useParamsStr()

@anteprimorac
Copy link
Contributor

If you have defined that your model has an ID that has type Int, then Prisma's typescript definition will warn you if you try to pass a string. That's in my opinion better DX then magical types conversion.

Prisma is doing the right thing there because having a strictly typed database layer is right, and this framework can benefit from it.

I thought that with useParams hook we are trying to resolve the issue with nextjs router that parses and combines query and params into the same object router.query. Additionally, I've tested a case when your param and query share the same key, for example, if you have route /projects/[id] and you try to access /projects/1?id=2 then your query id gets ignored, and router.query is { "id": "1" }.

Currently we have this code:

const router = useRouter();
const id = parseInt(router?.query.id as string);
const [user] = useQuery(getUser, { where: { id } });

and with useParams hook it could look like this:

const { id } = useParams();
const [user] = useQuery(getUser, { where: { id: Number(id) } });

If you have more complex params, you can use a library like yup and validate it before passing to a database query. In the future, blitz could provide params validation, sanitization and type conversion based on types from Prisma.

@flybayer
Copy link
Member

Yeah, unfortunately there's nothing we can do about the param/query conflict. That's deep inside Next.js itself. 😕

@anteprimorac
Copy link
Contributor

anteprimorac commented May 28, 2020

I was able to make a prototype for this.

// utils/router.ts
import { useRouter } from 'blitz';
import { parse } from 'url';
import { getRouteRegex, getRouteMatcher } from 'next/dist/next-server/lib/router/utils';

export const useRouterParams = () => {
  const router = useRouter();
  const { pathname } = parse(router.asPath);
  const params = getRouteMatcher(getRouteRegex(router.route))(pathname);
  if (params) {
    return params;
  }

  return {};
};

export const useRouterQuery = () => {
  const router = useRouter();
  const { query } = parse(router.asPath, true);
  return query;
};

You can test it by dropping it into utils. Example of use:

// app/users/pages/users/[id].tsx
import { useRouterParams } from 'utils/router';

export const User = () => {
  const { id } = useRouterParams();
  const [user] = useQuery(getUser, { where: { id: Number(id) } });
  return user;
};

@flybayer do you think if this is something that could be added to blitz core? I am happy to create PR.

@flybayer
Copy link
Member

Ok, here's some updated thoughts.

  • We have three types of params: string, number, and array
  • We want to solve a DX issue with having to manually parse numbers
  • We want to solve a DX issue with types (because values of router.query are string | string[])

Essentially we just need a filter that solves each case. We can do this with one single hook:

function useParams(type: 'string' | 'int' | 'array' = 'string') {
  // ...
}

// Return all string params — excludes arrays
const {zipcode} = useParams() 

// Return everything that passes parseInt — excludes everything else
const {taskId} = useParams('int')

// Return all array params — excludes everything else
const {slug} = useParams('array')

What do you all think of this?

@anteprimorac
Copy link
Contributor

anteprimorac commented May 28, 2020

I don't think that is intuitive to everyone because in that case output of that function depends on the type of param value. Also, I believe that validation, sanitization and casting of the user input should be more robust than just strings, integers and arrays and that it could be for any data handling for example forms validation.

What you think about adding another hook useParam(key: string, single: boolean = true): string|string[] that would return just a value of param key? You would be able to use it like this:

const id = parseInt(useParam('id'), 10); // number
const slugs = useParam('slug', false); // string[]
const categories = useParam('cat', false).map(cat => parseInt(cat, 10)); // number[]

@Zeko369 could you change the status of this issue to status/assigned?

@MrLeebo
Copy link
Member

MrLeebo commented May 29, 2020

The thing that really bothers me about it is that it still passively just nulls out values when you request something you expect to be a number but you get something else instead.

The ideal scenario, I think, would be if the type information was actually part of the route itself, so a type mismatch would simply prevent the route from matching at all. E.g. /projects/[id:number].tsx

@flybayer is that something that could be a NextJS enhancement, or possibly even a blitz rule?

@flybayer
Copy link
Member

flybayer commented May 29, 2020

@anteprimorac yeah, you're right — not super intuitive. We've come full circle — your last suggestion is basically the same as @Zeko369 very first post 😄

@MrLeebo that's something that could definitely be added to Next if their team is open to the idea. And yes, I think that probably could be added via Blitz. Might be tricky, but seems possible.

Regarding validation/sanitization: I totally agree we need proper validation/sanitization, but this needs to happen inside query/mutation functions. If you use a query to fetch an item and that item doesn't exist in the db, it will either throw a not found error or return a result that indicates the same (up to the developer on this).

All of the following should result in the same Not Found error:

/projects/100   (project with id=100 doesn't exist in DB)
/projects/hack  (hack not a number)

The thing that really bothers me about it is that it still passively just nulls out values when you request something you expect to be a number but you get something else instead.

So as long as you get the same end result, it doesn't matter how you get there in this case, right? All of the below approaches have the same result because id will be passed to getProject() where it will either find the item in the DB or it won't (or it won't pass input validation)

  • const projectId = parseInt(useParams().projectId)
  • const {projectId} = useParamsInt()
  • /projects/[projectId:number].tsx

So I do like /projects/[id:number].tsx and think it has potential, but unless I'm missing something I don't think we can automatically type the output of useParams() with that approach. Because we only know the type at build time, not statically.

I think we would need to statically define the types somewhere

// app/route-types.ts
export type = {
  projectId: number
  slug: string[]
}

@flybayer
Copy link
Member

flybayer commented Jun 2, 2020

Let's try to come to a conclusion here :)

Current:

const router = useRouter()
const taskId = Number(router?.query.taskId)
const zipcode = router.zipcode as string
const slug = router.slug as string[]

With blitz-js/blitz#574:

const params = useRouterParams()
const taskId = Number(params.taskId)
const zipcode = params.zipcode as string
const slug = params.slug as string[]

My final proposal for parsing & type guard:

const taskId = useParamNumber('taskId')   // return: number
const zipcode = useParamString('zipcode') // return: string
const slug = useParamArray('slug')        // return: string[]
  • useParamNumber will use Number(), so floats will be supported and it'll return NaN if parsing fails
  • useParamString and useParamArray don't parse, they only fix the typescript type.

@MrLeebo
Copy link
Member

MrLeebo commented Jun 3, 2020

I created vercel/next.js#13534 for the type-safe routing idea, but the proposed hooks can approximate that feature by redirecting to the catch-all route if the param has a type mismatch instead of returning NaN.

@anteprimorac
Copy link
Contributor

Actually, with blitz-js/blitz#574 the example should look like this:

const params = useRouterParams() // every param type is string | string[]
const taskId = Array.isArray(params.taskId)
  ? Number(params.taskId[0])
  : Number(params.taskId) // return: number
const zipcode = Array.isArray(params.zipcode)
  ? params.zipcode[0] 
  : params.zipcode        // return: string
const slug = Array.isArray(params.slug)
  ? params.slug
  : [params.slug]         // return: string[]

Your final proposal should resolve the Array.isArray check and parsing of number.

I've been experimenting with support for yup validation schema, and currently, it can be used like this:

type StatusFilter = 'active' | 'deleted';

const { status } = useRouterQuery({
  validateSchema: new yup.object().shape({
    status: yup
      .string()
      .oneOf<StatusFilter>(['active', 'deleted'])
      .default('active')
      .required(),
  }),
});

Currently, I have to pass schema to every hook call, but instead of that, it would be better to define it in the scope of the page(maybe pushing it into react context), and then our hooks can pull it from it.

What do you think about something like this?

@flybayer
Copy link
Member

flybayer commented Jun 4, 2020

@MrLeebo, that's a nice thorough feature request!

I think for now we need to stick with returning NaN from our hook because params will be empty during static prerendering. The actual error doesn't occur until runtime when we want to use that id to fetch data. If we through an error from the hook, then prerendering will fail.

@anteprimorac I'm still not sold on client-side url query validation (vs server-side), but you're definitely welcome to open a new issue for that!

@flybayer
Copy link
Member

flybayer commented Jun 4, 2020

@anteprimorac I unassigned you from this since blitz-js/blitz#574 doesn't solve this issue. But you're welcome to pick it back up if you want to also add the new param hooks.

@anteprimorac
Copy link
Contributor

anteprimorac commented Jun 5, 2020

@flybayer yes, I agree that doing validation only on client-side is not good, but I would like to explore how to share the same logic for URL validation on both sides.

You can assign this issue on me because I don't have permission to do it, and I will add a PR for adding new hooks for parsing single param/query.

What do you think about having just one hook for params and one for queries that would handle all these parsing cases?

const slug    = useParam('slug')              // returns: string|string[]
const taskId  = useParam('id', 'number')      // returns: number
const zipcode = useParam('zipcode', 'string') // returns: string
const slugs   = useParam('slug', 'array')     // returns: string[]

@flybayer
Copy link
Member

flybayer commented Jun 6, 2020

@anteprimorac I considered that, but unless I'm missing something it's impossible to type the result of that properly. (result is always number | string | string[] regardless of input)

@anteprimorac
Copy link
Contributor

@flybayer it's possible by using overloads.

@flybayer
Copy link
Member

flybayer commented Jun 6, 2020

Oh nice! Then lets do that

@anteprimorac
Copy link
Contributor

@flybayer what you think about adding the same functionality to useRouterQuery? We could just add additional arguments to existing hook that would give the ability to return specific query key cast to number, string or array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants