pbeshai / use-query-params

React Hook for managing state in URL query parameters with easy serialization.
https://pbeshai.github.io/use-query-params
ISC License
2.16k stars 96 forks source link

feature request: `strict` option to only include configured params in query object #150

Open tarekrached opened 3 years ago

tarekrached commented 3 years ago

First off, this package is incredible, and I am very grateful for it.

I have an app that has several sets of parameters that are of interest to different parts of the app. The general pattern is that the params are read all over the place, but there's one component for each of the sets of params that is responsible for writing that set of params.

The way I'm dealing with this is by having a couple of different useQueryParams that are fed the different configs, and it works great! Except that each of the useQueryParams attempts to parse all of the query params, not just those that are in it's paramConfigMap, so in dev mode, we get a bunch of warnings of the form:

Encoding parameter XXX as string since it was not configured

This is totally harmless and doesn't show up in prod, but it seems like it would be helpful to be able to restrict useQueryParams to only concern itself with the params in its paramConfigMap. I don't have a strong opinion about implementation - maybe a second strict parameter to useQueryParams that defaults to false? Something more elegant?

pbeshai commented 3 years ago

If you want to submit a PR that just removes that message, that's fine with me.

tarekrached commented 3 years ago

Right on. I'm not sure that I'd remove the warning message, though. The warning makes sense given the current behavior: useQueryParams adds all queryparams to the DecodedValueMap, regardless of whether they are in the paramConfigMap or not.

How would you view a PR adding a strict parameter to useQueryParams that defaults to false, which if true only adds the params in the paramConfigMap to the DecodedValueMap?

To be clear, I'm not wedded to this API shape, I'm just trying to be able to get a DecodedValueMap with the same entries as the paramConfigMap.

pbeshai commented 3 years ago

I think it's a reasonable option to have, but I don't want to add it at the moment. There are a few other options I'd like to support in the coming months that will likely get rolled up into a bigger release and I can include this in that list.

Although with regards to your initial message – why are you seeing an encoding error during parsing? That should only happen when calling setQuery on a non-configured parameter? I actually don't see the downside for the error message now that I'm reading your issue more carefully.

Are you perhaps doing something like:

const [query, setQuery] = useQueryParams({ ... });

setQuery({ ...query, changedParam: 'foo' })

?

If so, you can just do setQuery({ changedParam: 'foo' }) – the others are kept around by default (update types pushIn or replaceIn)

As for the downside of having extra values in your query object – is this due to iterating over the object? I suppose there is some overhead there, but I think the caching is set up in a way that it should be relatively negligible.

tarekrached commented 3 years ago

In terms of prioritization, that totally makes sense - no rush on my end. I agree that the overhead downside is probably negligible, my concern is more around the predictability of the shape of the DecodedValueMap.

...and after further investigation, the original warning was from a completely unrelated, misconfigured encodeQueryParams() in the same file as the partial useQueryParams 🤦.

I think I'd still make a strict mode for useQueryParams as a feature request, but super low priority. Feel free to close this issue or whatever.

Thanks again for your attention here!