Skip to content
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

fix(next-app-router): prevent client-side search when rerendering #6452

Merged
merged 2 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions packages/react-instantsearch-nextjs/src/InstantSearchNext.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { safelyRunOnBrowser } from 'instantsearch.js/es/lib/utils';
import { headers } from 'next/headers';
import { usePathname } from 'next/navigation';
import React, { useEffect, useRef } from 'react';
import {
InstantSearch,
Expand All @@ -20,9 +21,11 @@ import type {
} from 'react-instantsearch-core';

const InstantSearchInitialResults = Symbol.for('InstantSearchInitialResults');
const InstantSearchLastPath = Symbol.for('InstantSearchLastPath');
declare global {
interface Window {
[InstantSearchInitialResults]?: InitialResults;
[InstantSearchLastPath]?: string;
}
}

Expand All @@ -47,6 +50,17 @@ export function InstantSearchNext<
...instantSearchProps
}: InstantSearchNextProps<TUiState, TRouteState>) {
const isMounting = useRef(true);
const isServer = typeof window === 'undefined';
const pathname = usePathname();
const hasRouteChanged =
!isServer &&
window[InstantSearchLastPath] &&
window[InstantSearchLastPath] !== pathname;

// We only want to trigger a search from a server environment
// or if a Next.js route change has happened on the client
const shouldTriggerSearch = isServer || hasRouteChanged;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitializePromise and TriggerSearch wouldn't do anything on the client.

You don't need to have them to trigger a search on the client, normal CSR with 2 renders kicks in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means just having isServer is enough I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks in a dynamic route setting, where a route change caused by Next.js will unmount/remount InstantSearchNext, and not trigger the search.

Adding the check for path change allows <TriggerSearch> to be mounted and executed as you can see in this recording (the generated sandbox is failing to setup sadly).

I don't exactly understand how it kicks in. Probably Next.js route change performs similarly to router.refresh() and executes server components?

screenshot-20241129-102623.mp4

Copy link
Member

@aymeric-giraudet aymeric-giraudet Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah my bad sorry !
Actually I think this solution is nice, we could even choose not to delete initialResults and it would avoid that flash of no results. Not sure what the condition would look like though, but it can be done later in another PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And no I think it's because of something with InstantSearch RSC context that makes it not trigger the search even on the client.


useEffect(() => {
isMounting.current = false;
return () => {
Expand All @@ -55,6 +69,10 @@ export function InstantSearchNext<
};
}, []);

useEffect(() => {
window[InstantSearchLastPath] = pathname;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this also be set if someone has InstantSearch set up with routing for multiple different path names?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I get it, are you refering to a situation where there's an InstantSearch implementation in multiple paths and the user navigates from one to the other?

In that case, this is okay as search needs to be triggered.

In any case I'm not fond of having to store this data globally, but I haven't found a good way to track route changes since useRouter events are not available in app routing. I'm definitely open to ideas around that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of a use case where with routing someone has for example /search/:category set up as the path. If you refine the category, won't it also be true here and force an extra search?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change caters to that use case specifically. You can see it in this comment https://github.com/algolia/instantsearch/pull/6452/files#r1863228512.

Copy link
Member

@aymeric-giraudet aymeric-giraudet Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we had an InstantSearch singleton for React (that's what Apollo does iirc). It would make sure they only use one instance of InstantSearch and we could probably keep track of URLs in there.

}, [pathname]);

const nonce = safelyRunOnBrowser(() => undefined, {
fallback: () => headers().get('x-nonce') || undefined,
});
Expand All @@ -77,9 +95,9 @@ This message will only be displayed in development mode.`
<InstantSearchRSCContext.Provider value={promiseRef}>
<InstantSearchSSRProvider initialResults={initialResults}>
<InstantSearch {...instantSearchProps} routing={routing}>
{!initialResults && <InitializePromise nonce={nonce} />}
{shouldTriggerSearch && <InitializePromise nonce={nonce} />}
{children}
{!initialResults && <TriggerSearch />}
{shouldTriggerSearch && <TriggerSearch />}
</InstantSearch>
</InstantSearchSSRProvider>
</InstantSearchRSCContext.Provider>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/**
* @jest-environment jsdom
*/

import { createSearchClient } from '@instantsearch/mocks';
import { wait } from '@instantsearch/testutils';
import { act, render } from '@testing-library/react';
import React from 'react';
import { SearchBox } from 'react-instantsearch';

import { InstantSearchNext } from '../InstantSearchNext';

const mockPathname = jest.fn();
jest.mock('next/navigation', () => ({
...jest.requireActual('next/navigation'),
usePathname() {
return mockPathname();
},
}));

describe('rerendering', () => {
const client = createSearchClient();

function Component() {
return (
<InstantSearchNext searchClient={client} indexName="indexName">
<SearchBox />
</InstantSearchNext>
);
}

beforeEach(() => {
(client.search as jest.Mock).mockClear();
});

it('does not trigger a client-side search by default', async () => {
const { rerender } = render(<Component />);

await act(async () => {
await wait(0);
});

rerender(<Component />);

await act(async () => {
await wait(0);
});

expect(client.search).toHaveBeenCalledTimes(0);
});

it('triggers a client-side search on route change', async () => {
mockPathname.mockImplementation(() => '/a');
const { rerender } = render(<Component />);

await act(async () => {
await wait(0);
});

mockPathname.mockImplementation(() => '/b');
rerender(<Component />);

await act(async () => {
await wait(0);
});

expect(client.search).not.toHaveBeenCalledTimes(0);
});
});

afterAll(() => {
jest.resetAllMocks();
});
Loading