scottrippey / next-router-mock

Mock implementation of the Next.js Router
MIT License
400 stars 38 forks source link

Inconsistent useRouter behavior compared to the Next.js Router #87

Open illandril opened 1 year ago

illandril commented 1 year ago

If any of the router properties are accessed in a delayed fashion, if the route changes between the useRouter call and accessing the properties, then the original route's data is returned when using next-router-mock.

If the Next.js Router is used, the updated route's data is returned.

Example:

  const router = useRouter()
  useEffect(() => {
    const onRouteChange = (url) => {
      console.log('routeChangeComplete', router.query, router.asPath, url)
    }
    router.events.on('routeChangeComplete', onRouteChange)
    return () => {
      router.events.off('routeChangeComplete', onRouteChange)
    }
  }, [router])
  const routeChangeA = useCallback(() => {
    router.push('/path?x=A')
  })
  const routeChangeB = useCallback(() => {
    router.push('/path?x=B')
  })

Start at /path, then trigger routeChangeA and then routeChangeB. When using the Next.js Router, you'll get...

'routeChangeComplete' { x: 'A' } '/path?x=A' '/path?x=A'
'routeChangeComplete' { x: 'B' } '/path?x=B' '/path?x=B'

... but with next-router-mock you'll get...

'routeChangeComplete' {} '/path' '/path?x=A'
'routeChangeComplete' { x: 'A' } '/path?x=A' '/path?x=B'
scottrippey commented 1 year ago

I believe this is due to Next's async behavior. You can import from next-router-mock/async to ensure the async behavior is replicated. See: https://github.com/scottrippey/next-router-mock#sync-vs-async Please let me know if this works for you!

illandril commented 1 year ago

Nope - the behavior is the same for both sync and async.

I think the issue is because you're creating a snapshot of the router: https://github.com/scottrippey/next-router-mock/blob/d786607dbdc04819d37f4d9eaa32bec41c2a544e/src/useMemoryRouter.tsx#L15C1-L15C86

scottrippey commented 1 year ago

The snapshot is intentional, because that does seem to be how Next behaves. If you've got a closure around the router and you try to access the .query etc, it'll give you a snapshotted (stale) value. Here's a CodeSandbox to demonstrate:

scottrippey commented 1 year ago

Having said that, you're still observing a behavior that is different between next/router and next-router-mock, and I'd like to figure out what's going on.

Do you think you could create a CodeSandbox that demonstrates the Next behavior that you're seeing?

illandril commented 1 year ago

I forked your CodeSandbox and added a routeChangeComplete values section that demonstrates the behavior we're relying on: https://codesandbox.io/p/sandbox/holy-platform-kk7qfj

scottrippey commented 1 year ago

Thanks, now I see what's happening.
In Next, it appears that the routeChangeComplete event is triggering after React has re-rendered. Here's the order of events that I'm seeing:

  1. Initial render
  2. useEffect adds the routeChangeComplete event
  3. User event triggers the router.replace(...) call
  4. The component is re-rendered:
    • The useRouter hook returns a new snapshot of the router
    • This changes the useEffect dependencies, causing it to unsubscribe and resubscribe the routeChangeComplete event (with a new closure around the updated router
  5. The routeChangeComplete event is triggered, and successfully logs the updated router values

So here's where next-router-mock/async is behaving differently. The async version adds a setTimeout(0) before the routeChangeComplete event, in an effort to allow React to re-render.

It's possible/probable that this isn't good enough to allow React to rerender. I wonder if a longer timeout would help?

But also, it's possible that your test isn't allowing React to rerender. Are you using React Testing Library? If so, it can be tricky to properly use act to trigger a rerender. For example,

await act(() => {
  userEvent.click(container.getByText("Button 1"));
  userEvent.click(container.getByText("Button 2"));
});

This would trigger 2 router.push but it would only rerender once. You'd need to change that to:

await act(() => {
  userEvent.click(container.getByText("Button 1"));
});
await act(() => {
  userEvent.click(container.getByText("Button 2"));
});

I think it might be helpful, to move forward, if you could post some of your testing code.

scottrippey commented 1 year ago

There's also one more solution for you to consider. In Next, you can also use the "singleton router" for this kind of situation. The singleton router does not use snapshots, so reading the .query will always return the latest values. For example:

import router from 'next/router';

function ExampleComponent() {
  useEffect(() => {
    function onRouteChange() {
      // These values will always read the most up-to-date values:
      console.log(router.asPath, router.query);
    }
    router.on('routeChangeComplete', onRouteChange);
    return () => router.off('routeChangeComplete', onRouteChange);
  }, [] ); // Notice, no dependencies
}

This is a much better way to subscribe to router values, because you only need useRouter if you need the values during render. If you're only accessing router values during an event handler, then it's better to read them from the singleton.