47ng / nuqs

Type-safe search params state manager for Next.js - Like React.useState, but stored in the URL query string.
https://nuqs.47ng.com
MIT License
2.95k stars 62 forks source link

Testing components that use useQueryState? #259

Open rswerve opened 2 years ago

rswerve commented 2 years ago

I'm curious if anyone has written unit tests for a component using this library? I'm using it in a Next.js app (works beautifully, by the way) but when I try to test the component, it bombs out with "Cannot read property 'replace' of null" from this line:

https://github.com/47ng/next-usequerystate/blob/064ecfdaf093bbde2cc72db008685d4a588efed5/src/useQueryState.ts#L108

franky47 commented 2 years ago

I've been wondering the same thing about how to unit-test the library itself to be honest.

If anyone has suggestions, I'd be happy to add them to the README.

karuhanga commented 2 years ago

Unfortunately ended up rewriting this(here) to make use of router.query instead of window.location.search so that I could use https://www.npmjs.com/package/next-router-mock.

My initial hope was I'd just update the and do a PR, but there was some weirdness- likely with the useMemo optimizations. Perharps if someone threw more light on why we chose not to use router.query in the first place(only found the comment, but not much more context than that), I could take a stub on the PR again.

[great work btw :)]

franky47 commented 2 years ago

Thanks for your work and feedback @Karuhanga.

The reason why I didn't use router.query was for optimisation: I did not want useQueryState('foo') to cause rerenders when any other part of the router changed (path, unrelated query keys, hash etc..).

If this goes against the adage "make it work, make it right, make it fast, in that order", then I'd be happy to reconsider this decision.

Your comment raised an interesting idea: why stub the underlying implementation of this library (the need for a router) when we could instead provide a mock of useQueryState, where routing behaviour can be controlled and spied on from test code ? Would this be an interesting approach ?

karuhanga commented 2 years ago

The reason why I didn't use router.query was for optimisation: I did not want useQueryState('foo') to cause rerenders when any other part of the router changed (path, unrelated query keys, hash etc..).

I see. I think it might be possible to prevent these rerenders with a strict memoization, maybe based on router.query.key?

Your comment raised an interesting idea

Yes. That was actually something I considered(e.g using useState as a mock impl in tests). It might also better since the router doesn't have a hash component yet anyway. I don't see any big reason that wouldn't work, I was already using the mock anyway so I went that direction. I suspect most people are already using that mocking package as well, so maybe something else to think about is if we want to expand the scope of this package. I liked that it was small and neat.

Generally though, I think depending on two different sources of truth for what the queries are is not great.

leon486 commented 1 year ago

Can you provide information about how you ended up testing this? I'm having the same problem and rewriting the package itself is something I would prefer to avoid.

I'm curious if anyone has written unit tests for a component using this library? I'm using it in a Next.js app (works beautifully, by the way) but when I try to test the component, it bombs out with "Cannot read property 'replace' of null" from this line:

https://github.com/47ng/next-usequerystate/blob/064ecfdaf093bbde2cc72db008685d4a588efed5/src/useQueryState.ts#L108

franky47 commented 1 year ago

As I'm working on shipping an update that supports the app router, I'd like to collect some feedback and ideas on how to make it easier to test these hooks.

In the upcoming API, the setState query updater function will return a Promise to the updated URLSearchParams. This might make it easier to unit-test the hook itself, but won't be much help in a synchronous component.

For testing components that use the hook, a black box approach would be recommended:

  1. Trigger an action in the component (eg: click a button). That action calls a query state updater function.
  2. Assert that the state update had an effect on the component (eg: changing text or updating the DOM)
  3. Assert that the URL has changed accordingly

Points 1 and 2 are akin to testing regular React.useState hooks (which is exactly what there is under the hood now), so let's focus on testing point 3.

Since URL updates are now asynchronous to support batching and throttling, it may become more difficult to test. However, as the library uses an event emitter internally, we could expose a special key that emits URLSearchParams when updates are applied to the URL. Test code could subscribe to this key using mock callbacks, and assert that they have been called with certain expected values.

On the other hand, the initial issue had to do with testing the hook outside of a Next.js router context. It should [^1] now be possible to rely only on the test environment providing a Web History API mock, which could also be hooked for assertions.

Edit: initial attempts to mount a component using useQueryState in a RTL test fixture failed due to the dependency on useSearchParams and useRouter failing to find a Next.js router context. This would require mocking the router, using next-router-mock, for which app router support is being worked on.

Notes: some useful links to how to mock next/navigation:

[^1]: The hooks still rely on the Next.js router in the app directory for providing server-side access to the querystring via useSearchParams. Testing the server-side behaviour of the hooks vs the client side (depending on whether the test environment exposes window) seems like a mine field to me..

sgoodrow-zymergen commented 6 months ago

Any workarounds in the meantime? In particular, I'd like to be able to use next-router-mock to initialize a url (and search query param) and have nuqs deserialize its state from it.

ivasilov commented 4 weeks ago

Just another issue to add:

 ⨯ Error: invariant expected app router to be mounted
    at useRouter (/Users/ivasilov/Documents/supabase/node_modules/next/dist/client/components/navigation.js:157:15)
    at useQueryState (file:///Users/ivasilov/Documents/supabase/node_modules/nuqs/dist/index.js:305:18)

This is on a pages router in a page with no SSR functions defined (getServerSideProps etc).

I've localized the issue to NODE_ENV=test env var. The variable is used in Playwright tests (to force the Nextjs app to use env.test file), which should be ok according to Nextjs docs. It's happening probably because of the import of next/navigation.js.

PavelYatskevich commented 3 weeks ago

I have similar issue to previous comment

image

We currently migrating from jest to vitest and getting this error atm, maybe some on faced same issue, thanks