use-query-params icon indicating copy to clipboard operation
use-query-params copied to clipboard

Param types without null?

Open fgblomqvist opened this issue 4 years ago • 6 comments

All of the official types are null-friendly. E.g. the type of a simple parameter such as a BoolParam is bool | null | undefined. I'm guessing (based on the docs) that the purpose of this is to allow people to pass &foo and it getting parsed rather than thrown out.

For a more advanced parameter such as ArrayParam, it is (string | null)[] | null | undefined. This is (probably) because people can pass &foo&foo=x&foo etc. which would result in [null, 'x', null]. This is slightly trickier to work with (you have to do a filter to get rid of all null values rather than just a coalesce). I'm not sure when this value would just become null though (as in, imo the correct type would be (string | null)[] | undefined.

Anyway, I'm not sure I understand the value in "letting through" null values (from a default perspective)? What is the motivation here? To be as 1:1 as possible with the URL params? Have you ever considered a strict mode that skips null values in all types? Or rather, strict param types I guess. I know I can just create these types myself, but it's more interesting to hear the general argument for when null is useful (for the average user).

This is a very handy little library, and I appreciate all the work you've put into it no matter what! 🙂

fgblomqvist avatar Jun 04 '21 15:06 fgblomqvist

Personally I never want null in query params and only added in support for them for other folks who requested it. I can think of an example usage where maybe you are configuring a 3 column layout and you want to say what's in each spot where one [A, null, C] if you haven't put one in the middle. I think this came up at some point.

I am planning on a newer release with a shared options object that would allow you to configure at the top level your desired null behavior. For me currently, I always do things like:

export const ArrayParam = withDefault(
  DefaultArrayParam,
  undefined
) as QueryParamConfig<string[] | undefined>

export const BooleanParam = withDefault(
  DefaultBooleanParam,
  undefined
) as QueryParamConfig<boolean | undefined>

which just ignores the (string|null)[] case and otherwise replaces the null with undefined. If someone messes with the URL to put a null in the middle of the array and it breaks, I don't care 😅

As for your thoughts on the correct type for ArrayParam, consider these possibilities:

?foo=A&foo=B // ['A', 'B'] -> string[]
?foo&foo=A // [null, 'A'] -> (string|null)[]
?foo= // [] -> Array
?foo // null
? // undefined

pbeshai avatar Jun 04 '21 15:06 pbeshai

That being said, I believe currently you can pass in stringifyOptions to include skipNull: true. See https://github.com/sindresorhus/query-string#skipnull

It wouldn't fix the types, not sure if I'd ever manage that 😓

<QueryParamProvider
  ReactRouterRoute={Route}
  stringifyOptions={{ skipNull: true }}
>
  <App />
</QueryParamProvider>

Although I'm not sure they are used on the parse side.

pbeshai avatar Jun 04 '21 15:06 pbeshai

As for your thoughts on the correct type for ArrayParam, consider these possibilities:

?foo=A&foo=B // ['A', 'B'] -> string[]
?foo&foo=A // [null, 'A'] -> (string|null)[]
?foo= // [] -> Array
?foo // null
? // undefined

?foo should translate to [null] because you have specified that you're looking for an array, so therefore it should be parsed as such at all times. It would be quite chaotic in the programming world if doing const x = ['foo']; resulted in x = 'foo'. Just my 2 cents 😛

Personally I never want null in query params and only added in support for them for other folks who requested it. I can think of an example usage where maybe you are configuring a 3 column layout and you want to say what's in each spot where one [A, null, C] if you haven't put one in the middle. I think this came up at some point.

Sounds like a typical case of optimizing for a niche case 🙈 . I can totally see that being a valid use-case, but certainly not a common one.

Your suggestion on the custom Params make sense, just wish I wouldn't have to do all those redefinitions (but might be the only way out for now). Will look into setting stringifyOptions={{ skipNull: true }}. Seems slightly better but yeah I'd still have to recast all the params to fix the types.

My proposed solution would probably be to make a breaking update and export all the current types as NullableBoolParam, NullableArrayParam etc.. and then export non-nullable BoolParam etc.. (which I guess could just be your sorta make-shift solution you showed with withDefault 🤷

Thanks for your quick response!

fgblomqvist avatar Jun 04 '21 15:06 fgblomqvist

These cases are not currently possible with the current implementation:

['']   // would be ?foo=
[null] // would be ?foo

Alternatives would be to disallow:

[]   // currently ?foo=
null // currently ?foo

Trade-off that seems more consistent with the other params, but neither are perfect. At least people can make their own array param to do what they prefer.

As for optimizing for a niche case, I think at the time I needed to change some internals to actually allow nulls to propagate through to the params themselves. It, at the time, wasn't even possible to get nulls as a user. Now I see I could've made the params that ship with the lib still strip them out, and maybe that's a reasonable choice. Then others could make their own params that include them, or I could ship more params and hope tree shaking works. I like the idea of Nullable*Param.

It's likely in the next version I'll do things that I think are more convenient by default while hopefully still making it possible for more niche use cases. Hope to get around to it soonish.

pbeshai avatar Jun 04 '21 16:06 pbeshai

hope tree shaking works

always have faith 😎 (if it doesn't work, it's probably not your fault)

But yeah sounds good!

fgblomqvist avatar Jun 04 '21 17:06 fgblomqvist

hope tree shaking works

Just don't rely on it, and export only 1 thing from a file. It will never work 100%, you can see the explanation in the webpack docs why that is.

dietergeerts avatar Jul 06 '21 14:07 dietergeerts