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

useParam is better to return undefined when the param is missing #616

Closed
isoppp opened this issue Nov 7, 2020 · 6 comments · Fixed by blitz-js/blitz#1474
Closed

useParam is better to return undefined when the param is missing #616

isoppp opened this issue Nov 7, 2020 · 6 comments · Fixed by blitz-js/blitz#1474

Comments

@isoppp
Copy link
Contributor

isoppp commented Nov 7, 2020

What do you want and why?

const query = {}
cosnt params = useParams() // returns {}
cosnt params = useParams('number') // returns {}
const param = useParam('key', "number") // returns NaN
  1. I want to check param like if(param) but NaN will be true. It might not be useful.
  2. Between useParams and param is not consistent, I'm a bit confusing.
  3. Type implementation is number | undefined, but never returned undefined I guess

https://github.com/blitz-js/blitz/blob/canary/packages/core/src/use-params.ts#L92

export function useParam(key: string, returnType: "number"): number | undefined`

Possible implementation(s)

Want to change these test results to be undefined.

useParam("doesnt-exist", TYPE) in https://github.com/blitz-js/blitz/blob/canary/packages/core/test/use-params.test.ts

or

change type definition such as number | undefined to be number if it truly never returned undefined

Additional context

When first render, useRouter().query is always return empty object {} as mentioned in #962 #1334, this modification might be helpful.

@flybayer
Copy link
Member

flybayer commented Nov 7, 2020

@isoppp yeah, you have a great point.

We should change useParam to always return undefined if the requested param doesn't exist. This will be a change to all three types, string, number, and array.

@flybayer flybayer added the good first issue Good for newcomers label Nov 7, 2020
@HendrikCromboom
Copy link

To pull from some of the more back end minded techs:

I always loved the: mixedType (number OR undefined)

That way you could inject the typed return value rather than hardcoding it.

@MrLeebo
Copy link
Member

MrLeebo commented Nov 9, 2020

We've talked about the implications of type checking URL parameters before, but now that nextjs supports rewrites, we could revisit the idea of defining a type mapping for parameters and lettings blitz generate the corresponding rewrites to enforce it. That would make it impossible for the param to have an invalid value in the first place.

@flybayer
Copy link
Member

@MrLeebo regardless if we add that, there will still be the case of undefined for static pre-rendering and first render, which this issue is mainly about.

@KATT
Copy link
Contributor

KATT commented Nov 17, 2020

I did a PR above, but still remaining issue in regards to NaN - I personally really dislike NaN and always prefer to throw, or return undefined, or return null whenever I parse numbers

@KATT
Copy link
Contributor

KATT commented Nov 17, 2020

However, @isoppp, it's not great / strictly correct to do if (num) as 0 is falsy - better would be to if (typeof number === 'number').

... in this case if (typeof num === 'number' && !isNaN(num) 🥴

For reference - this is the fun number parsing I use in my project for parsing query params:

export function isNumeric(value: string) {
  return /^[+-]?\d+(\.\d+)?$/.test(value);
}

export function parseFloatOrNull(value?: unknown): number | null {
  if (typeof value === 'string' && isNumeric(value)) {
    const float = parseFloat(value);
    if (!Number.isNaN(float)) {
      return float;
    }
  }
  return null;
}

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.

5 participants