blitz-js / legacy-framework

MIT License
3 stars 2 forks source link

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

Closed isoppp closed 3 years ago

isoppp commented 4 years ago

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 blitz-js/legacy-framework#962 #1334, this modification might be helpful.

flybayer commented 3 years ago

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

HendrikCromboom commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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;
}