solidjs / solid-router

A universal router for Solid inspired by Ember and React Router
MIT License
1.13k stars 143 forks source link

`useSearchParams` suggestion #328

Open rattleSSnake opened 1 year ago

rattleSSnake commented 1 year ago

It would be very needed that setSearchParams from solid-start works like setting state, meaning it holds previous value and overwrites existing state when omitting prev. That way, it both works when you want to overwrite existing or append.

setSearchParams({ hello: "world" }) //overwrites existing
setSearchParams(prev => ({...prev, hello: "world" })) //appends
mtt-artis commented 1 year ago

Undefined for delete ?

setSearchParams(prev => ({...prev, hello: undefined })) // delete
rattleSSnake commented 1 year ago

If searchParams returned an Object instead of a Proxy, one could do:

setSearchParams(prev => {
  delete prev["hello"]
  return prev
})

It should return an object imo:

export function useSearchParams() {
  let initial = new URLSearchParams(isServer ? {} : window.location.search);

  const params = () => {
    const obj = {};
    for (const v of initial.keys()) obj[v] = initial.get(v);
    return obj;
  };

  const add = obj => {
    for (const key in obj) initial.append(key, obj[key]);
    const search = initial.length ? `?${initial.toString()}` : "";
    window.history.replaceState(null, "", window.location.pathname + search);
  };

  const setParams = arg => {
    if (typeof arg === "object") {
      initial = new URLSearchParams(); //reset
      add(arg);
    } else if (arg instanceof Function) {
      const old = params();
      initial = new URLSearchParams(); //reset
      add(arg(old));
    }
  };

  return [params, setParams];
}
mtt-artis commented 1 year ago

i think proxy is needed for reactivity the current useSearchParams function could be updated didn t try tbh

https://github.com/solidjs/solid-router/blob/HEAD/src/routing.ts#L100-L115

export const useSearchParams = <T extends Params>(): [
  T,
  (newParams: SetParams | ((current: Readonly<T>) => SetParams), options?: Partial<NavigateOptions>) => void
] => {
  const location = useLocation();
  const navigate = useNavigate();
  const setSearchParams = (params: SetParams | ((params: Readonly<T>) => SetParams), options?: Partial<NavigateOptions>) => {
    const searchString = typeof params === "function"
      ? untrack(() => mergeSearchString("", params(location.query as T)))
      : untrack(() => mergeSearchString(location.search, params));
    navigate(location.pathname + searchString + location.hash, {
      scroll: false,
      resolve: false,
      ...options
    });
  };
  return [location.query as T, setSearchParams];
};
rattleSSnake commented 1 year ago

But since it is a proxy, how would you deal this situation ?

setSearchParams(prev => {
  delete prev["something"];
  console.log(prev)
  return prev
});

Also on this line it should be:

: untrack(() => mergeSearchString("", params));

to make overwrite work

mtt-artis commented 1 year ago

you might be able to use produce https://www.solidjs.com/docs/latest/api#produce again, i didn t try


: untrack(() => mergeSearchString("", params));

would be a breaking change

rattleSSnake commented 1 year ago

You think it's ok to design it like that ? The user should be able to delete key in this way imo:

setSearchParams(prev => {
  delete prev["something"];
  console.log(prev)
  return prev
});
mtt-artis commented 1 year ago

I don't really like the delete keyword in general so this doesn't bother me but that's a personal preference and I'm not a core member ;)

I think Ryan spoke about this pattern for store in one of his streams. I'll add the link if I find it.

rattleSSnake commented 1 year ago

Thank you for your contribution @mtt-artis. I believe you didn't include this feature that could be a breaking change.

: untrack(() => mergeSearchString("", params));

would be a breaking change

setSearchParams should behave like state, where a plain object would overwrite existing params.

setSearchParams({ lonelykey: "i-overwrite" });

Imo if we don't want to change this there is no point to add callback

mtt-artis commented 1 year ago

Hey @rattleSSnake I share the logic here but I don't think the breaking change is a good idea. Noneless I'd be happy to make the change and refactor mergeSearchString if the reviewer asks me to do so.

rattleSSnake commented 1 year ago

With the update, can we somehow reset search params (by deleting every previous key I guess) ?

What happens if I do this: setSearchParams(prev => ({ omitprev: "what" }));

mtt-artis commented 1 year ago

This will reset search params

setSearchParams(prev => ({ }));
chiefcll commented 2 hours ago

Just to add in to this - I'm looking to support Browsers before Proxy support is added. I'd prefer for it also to be a plain Object. If there is any way to do this without proxy (maybe defining which search or query params one was looking for). I'll be diving into Router code this week probably forking it to remove that for now as I don't need reactive params.