Blitz: useParam is better to return undefined when the param is missing

Created on 7 Nov 2020  路  6Comments  路  Source: blitz-js/blitz

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 #1344 #1334, this modification might be helpful.

good first issue kinfeature-change statudone

Most helpful comment

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.

All 6 comments

@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.

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.

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.

@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.

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

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;
}
Was this page helpful?
0 / 5 - 0 ratings

Related issues

timsuchanek picture timsuchanek  路  5Comments

simonedelmann picture simonedelmann  路  3Comments

MrLeebo picture MrLeebo  路  5Comments

flybayer picture flybayer  路  3Comments

netheril96 picture netheril96  路  4Comments