-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Add atomWithQueryParam #34
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
I think you misunderstood #20 (as I did at first.) What's proposed in this PR is a wrapper around atomWithLocation for searchParams. It doesn't seem to add much capability other than serialization. There might be some other shape of utils, instead of having atomWithLocation in it. @Flirre might have some other thoughts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great addition to jotai-location
even if it's unrelated to #20. Like a searchParam counterpart to atomWithHash
.
I'm not entirely sure about the name, maybe it should be something like atomWithSearchParam
?
I like that there are tests added, but I think they could be written a bit more verbose so it's easier to follow at a glance.
If you're up for it I would really appreciate tests where you're using several searchParams to ensure that it handles that as well.
const { result } = renderHook(() => useAtom(queryParamAtom), { | ||
// the provider scopes the atoms to a store so their values dont persist between tests | ||
wrapper: Provider, | ||
}); | ||
|
||
act(() => { | ||
result.current[1]({ value: 'test value' }); | ||
}); | ||
|
||
expect(result.current[0]).toEqual({ value: 'test value' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like these tests, but feel that they could be a bit more verbose and/or easy to read. I feel tests can help act as a sort of documentation in a way.
What follows is just a quick rewrite as an example, I haven't tested it out locally.
const { result } = renderHook(() => useAtom(queryParamAtom), { | |
// the provider scopes the atoms to a store so their values dont persist between tests | |
wrapper: Provider, | |
}); | |
act(() => { | |
result.current[1]({ value: 'test value' }); | |
}); | |
expect(result.current[0]).toEqual({ value: 'test value' }); | |
const { result } = renderHook(() => useAtom(queryParamAtom), { | |
// the provider scopes the atoms to a store so their values dont persist between tests | |
wrapper: Provider, | |
}); | |
const [testValue, setTestValue] = result.current; | |
act(() => { | |
setTestValue({ value: 'test value' }); | |
}); | |
expect(testValue).toEqual({ value: 'test value' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const [testValue, setTestValue] = result.current;
i would expect this change to break the test actually because RTL updates result.current with the new testValue but that wouldn't rerun the destructuring after the act call.
That's the reason for the unfortunate index access syntax (which Jotai even has in it's docs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw we've built a helper wrapper internally that i think makes this a little nicer. I'd love to contribute it back to the community since it cleans up the readability of these tests like you were hoping, but I'm not sure to which library this util would belong tbh
const HOOK_STATE = 0;
const HOOK_SET_STATE = 1;
type RefStableState<V, F extends (...args: Array<any>) => void> = {
getState: () => V;
setState: (...args: Parameters<F>) => void;
};
/**
* When given a ref that contains the standard react state API: [state, setState]
* gives back a tuple with a getState and setState function instead that are correct as the ref is updated
* and can be used like
* ```
* const { result} = renderHook(() => useState('something'), {
wrapper: getProvideUserContext(),
})
* const {getState, setState} = stateFromRef(result);
expect(getState()).toEqual('something');
* ```
*/
export function getRefStableState<
V,
F extends (...args: Array<any>) => void,
>(ref: { current: [V, F] }): RefStableState<V, F> {
return {
getState() {
return ref.current[HOOK_STATE];
},
setState(...args: Parameters<F>) {
ref.current[HOOK_SET_STATE](...args);
},
};
}
/**
* When given a React Testing Library hook result whose return value follows a similar pattern to the react state API,
* returns a similar looking object but with the result key wrapped with stateFromRef
* This is useful when you want to test a hook that returns a state and setState tuple,
* but you want to use the getState and setState functions.
*
* Like this:
* ```
* const { result } = await wrapStateLikeHook(
* renderHook(() => useAtom(atomBiQueryFilters), {
* wrapper: getProvideUserContext(),
* })
* );
* await act(async () => {
* result.setState('something');
* });
* expect(result.getState()).toEqual('something');
* ```
*/
export async function wrapStateLikeHook<
R extends [any, (...args: Array<any>) => any],
P,
>({ result, ...rest }: RenderHookResult<R, P>) {
// this is null until done suspending
await waitFor(() => !!result.current[0]);
const refState = getRefStableState(result);
return {
...rest,
result: refState,
};
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice moving these out to a util file 👌
Please format the file with something like prettier
to get rid of the error.
One thing about this, which we couldn't find a clear answer to (@fravic and I have discussed offline), was how to update the atom's state when a query param changes independently, via push / replace state for example. Any ideas on that would be great to hear. The hash atom uses hashchange event but as far as I could find there's no corresponding event for the location.search property.. :/ |
Good question, but sadly I don't have any real answers. If I remember correctly you can see in the Another thing that could work in the future when supported by all browsers would be the navigate event. But I haven't tried an implementation of this, but it should be possible to test since Chrome supports it at the moment. Have you looked at any of this for inspiration? |
Thanks for taking a look at this all! The navigate events look great, thanks for pointing that out @Flirre. It does indeed seem to work great in Chrome, but probably best not to rely on it until it has full browser support. I think we'll go with a NextJS-specific query param atom internally, so I'll go ahead and close this for now given the difficulty around subscribing to query param changes |
It seemed I was able to get this working by accessing the query params by using window.location instead of using the atomWithLocations. I'm still using the atomWithLocation to set/get the state though. Are their potential issues that could come up from doing this that you may see? I was hoping to implement it into my teams code base.
|
Yes window.location isn't reactive so the atom value isn't going to update if someone update the location.search property from outside the atoms (eg a relative page link, or the next router etc) |
Related to issue: #20
Hello! Here's my attempt at an atom for a search/query param. The API attempts to be similar to
atomWithHash
andatomWithLocation
. Would appreciate your review/thoughts!