search

solidjs / solid-router

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

feat: extend useSearchParams hook #271

Closed mtt-artis closed 6 months ago

mtt-artis commented 1 year ago

feat: Enable passing a function to setSearchParams for dynamic search updates

This commit introduces a new enhancement to the useSearchParams hook, allowing the setSearchParams function to accept a callback function as an argument. By passing a function instead of a plain object to setSearchParams , developers can now perform dynamic search updates based on the current search parameters. The callback function receives the existing search parameters as its argument, and it should return an updated search parameters object. This feature enables more flexible and dynamic search functionality, empowering developers to easily handle complex search scenarios and update search parameters in a controlled manner.

https://github.com/solidjs/solid-router/issues/328

ryansolid commented 1 year ago

I like the idea here. Only question. I noticed that this doesn't merge with the existing location.search. Is there a reason that passing the function form would have different expectations on what the returned value does?

mtt-artis commented 1 year ago

Hello @ryansolid 👋 I'm not sure I understand the question (sorry for my bad English) I made this PR after reading this issue https://github.com/solidjs/solid-router/issues/328 I'm on holiday without my pc right now but I can check the merge conflict this weekend

ryansolid commented 7 months ago

I see the problem now. This was always a merge instead of a set. Which is a bit awkward. Hence why you made the function a set rather than a merge, because the consumer could always do the merge as they saw fit. But it's inconsistent.

Realistically it should have never been merge I think. Solid Stores blur the line but I think it is confusing in general. However, if it weren't a merge the function form would have always been the one that would have made sense. Ironically I probably wanted it to merge originally for ease of use.

In any case, this probably should go one way or the other.