algolia / instantsearch

⚡️ Libraries for building performant and instant search and recommend experiences with Algolia. Compatible with JavaScript, TypeScript, React and Vue.
https://www.algolia.com/doc/guides/building-search-ui/what-is-instantsearch/js/
MIT License
3.71k stars 520 forks source link

NextJS `router.query` is stale after hitting back button #6002

Open avremel opened 9 months ago

avremel commented 9 months ago

🐛 Current behavior

If you hit a filter value, router.query gets populated correctly with the new URL. If you hit the browser back button, the facet selection gets cleared but router.query still has the old filter selection and the component doesn't even get re-rendered.

🔍 Steps to reproduce

  1. Click on any Brand filter.
  2. In dev tools you will see router.query being logged with the brand filter.
  3. Hit the back button.
  4. In dev tools you won't see anything logged because router.query hasn't been updated.

Live reproduction

https://codesandbox.io/p/devbox/nextjs-routing-issue-x9p8gy

💭 Expected behavior

I expect:

  1. router.query to have a new value
  2. That new value to be an empty object
  3. The component to re-render and trigger a log of router.query

Package version

algoliasearch 4.14.3, react-instantsearch 7.5.1, NextJS 12.3.4

Operating system

Mac 11.6.5

Browser

Chrome 113.0.5672.126

Code of Conduct

Haroenv commented 9 months ago

Is this the same as #5987? If you patch InstantSearch to use history.pushState instead of the next router, does it work?

avremel commented 9 months ago

@Haroenv I'm using NextJS 12.3.4, which only has a page router.

How can I patch it to use history.pushState? The same as the Remix example in the docs?

Haroenv commented 9 months ago

Ah I misunderstood the problem, that's indeed not related. If you add a button unrelated to InstantSearch that pushes to the router like this (https://codesandbox.io/p/devbox/nextjs-routing-issue-forked-t7w35v?file=%2Fpages%2Findex.tsx%3A60%2C1-64%2C16):

      <button type="button" onClick={() => {
        router.push('?something=true', undefined, {shallow: true})
      }}>
        add to url
      </button>

It behaves the exact same. I believe because the push is shallow, next doesn't consider that the router and app component needs to re-render, but only what has listeners to the beforePopState router event.

What use case do you have for having the router being updated, does it cause downstream issues? I wonder if something can be said to the router in the beforePopState function (https://github.com/algolia/instantsearch/blob/bbb27fc8839e7b775b599f411f6e8336771b0466/packages/react-instantsearch-router-nextjs/src/index.ts#L147) à la "there was an update, please render root again", but not sure if that's possible. Maybe emitting a routeChangeComplete is possible, but I wouldn't bet on it.

I don't believe the router should be "not shallow", as then the backend gets fetched, which is definitely a waste of resources.

avremel commented 9 months ago

@Haroenv I'll give you an example from our app:

  1. Navigate to https://www.ajmadison.com/c/dryers/
  2. The title is Dyers
  3. Click on Fuel Type: Electric
  4. The tile Electric Dryers
  5. Hit the back button
  6. The title is still Electric Dryers

We use router.query to calculate the title, that's why we can't rely on a shallow update. Same goes for which facets are shown on the page, they are dependent on the current route.

What would the alternative be? Should I be checking useInstantSearch instead?

Would it be possible add an option for shallow: false?

Haroenv commented 9 months ago

I'm not convinced shallow: false would even solve the issue if that's what's going on, however using useInstantSearch definitely will be updated with the ui at all times, so I recommend you use that for your source of truth instead of router. Does that work for you?

avremel commented 9 months ago

I will try using useInstantSearch. If I'm not mistaken, I had issues relying on it early on, which is why I resorted to router.query. I'll give it another try.

Haroenv commented 9 months ago

I guess an issue you may have is if it's inside InstantSearch, there may be a chance of a render where the component doesn't render (although I think that's just a client rendering potential problem, shouldn't happen when you server side render)