Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

fix(hooks): compute initial search parameters from widget #3399

Merged
merged 1 commit into from
Mar 28, 2022
Merged

fix(hooks): compute initial search parameters from widget #3399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions packages/react-instantsearch-hooks/src/connectors/__tests__/useClearRefinements.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ describe('useClearRefinements', () => {

// Initial render state from manual `getWidgetRenderState`
expect(result.current).toEqual({
hasRefinements: false,
canRefine: false,
hasRefinements: true,
canRefine: true,
refine: expect.any(Function),
createURL: expect.any(Function),
});
Expand Down
19 changes: 17 additions & 2 deletions packages/react-instantsearch-hooks/src/connectors/__tests__/useCurrentRefinements.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,23 @@ describe('useCurrentRefinements', () => {

// Initial render state from manual `getWidgetRenderState`
expect(result.current).toEqual({
items: [],
canRefine: false,
items: [
{
attribute: 'brand',
indexName: 'indexName',
label: 'brand',
refine: expect.any(Function),
refinements: [
{
attribute: 'brand',
label: 'Apple',
type: 'disjunctive',
value: 'Apple',
},
],
},
],
canRefine: true,
refine: expect.any(Function),
createURL: expect.any(Function),
});
Expand Down
33 changes: 30 additions & 3 deletions packages/react-instantsearch-hooks/src/hooks/__tests__/useConnector.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ const connectCustomSearchBox: Connector<
query: searchParameters.query,
};
},
getWidgetSearchParameters(searchParameters, { uiState }) {
return searchParameters.setQueryParameter('query', uiState.query || '');
},
};
};

Expand Down Expand Up @@ -223,7 +226,15 @@ describe('useConnector', () => {

function SearchProvider({ children }) {
return (
<InstantSearch searchClient={searchClient} indexName="indexName">
<InstantSearch
searchClient={searchClient}
indexName="indexName"
initialUiState={{
indexName: {
query: 'query',
},
}}
>
<InstantSearchContext.Consumer>
{(searchContextValue) => {
searchContext = searchContextValue;
Expand All @@ -247,9 +258,25 @@ describe('useConnector', () => {
wrapper: SearchProvider,
});

const helperState = {
disjunctiveFacets: [],
disjunctiveFacetsRefinements: {},
facets: [],
facetsExcludes: {},
facetsRefinements: {},
hierarchicalFacets: [],
hierarchicalFacetsRefinements: {},
index: 'indexName',
numericRefinements: {},
query: 'query',
tagRefinements: [],
};

expect(getWidgetRenderState).toHaveBeenCalledTimes(1);
expect(getWidgetRenderState).toHaveBeenCalledWith({
helper: expect.any(Object),
helper: expect.objectContaining({
state: helperState,
}),
parent: indexContext!,
instantSearchInstance: searchContext!,
results: expect.objectContaining({
Expand All @@ -263,7 +290,7 @@ describe('useConnector', () => {
helper: expect.any(Object),
},
],
state: expect.any(Object),
state: helperState,
renderState: searchContext!.renderState,
templatesConfig: searchContext!.templatesConfig,
createURL: indexContext!.createURL,
Expand Down
14 changes: 8 additions & 6 deletions packages/react-instantsearch-hooks/src/hooks/useConnector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,17 +89,19 @@ export function useConnector<
if (widget.getWidgetRenderState) {
// The helper exists because we've started InstantSearch.
const helper = parentIndex.getHelper()!;
const uiState = parentIndex.getWidgetUiState({})[
parentIndex.getIndexId()
];
helper.state =
widget.getWidgetSearchParameters?.(helper.state, { uiState }) ||
helper.state;
Comment on lines +95 to +97
Copy link
Contributor

Choose a reason for hiding this comment

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

this means that if this part of the function gets called, but the widget never actually mounts, there will be stray search parameters not associated to a widget (as we don't call dispose). Could this ever be a real issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why wouldn't the widget mount at this point?

And getWidgetSearchParameters() returns a value without triggering side effects, so I'm not sure to understand what you're saying.

Copy link
Contributor

Choose a reason for hiding this comment

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

getWidgetSearchParameters doesn't have any side-effects, but helper.state = getWidgetSearchParameters() is a side-effect.

I'm not sure why it wouldn't be mounted, but I'm not 100% sure whether the contract of react allows a memo to be called before/after the component is already mounted? Are there any specs of that? Is it possible a memo is called without the rest of the component being called?

Copy link
Member Author

Choose a reason for hiding this comment

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

Assigning helper.state is indeed a side effect. I think it's localized in that case because each widget overrides it for its initial rendering, and then the InstantSearch.js lifecycle takes over and sets it.

React can call memo anytime that it wants to call it (for memory/optimization purposes), so there's no certainty that it'll be called only once. I don't think that the new code introduced competes with the existing logic of useConnector() and useMemo() though. We already had this consideration before this change.

const results =
// On SSR, we get the results injected on the Index.
parentIndex.getResults() ||
// On the browser, we create fallback results based on the widget's
// `getWidgetSearchParameters()` method to inject the initial UI state,
// or fall back to the helper state.
createSearchResults(
widget.getWidgetSearchParameters?.(helper.state, {
uiState: parentIndex.getWidgetUiState({})[parentIndex.getIndexId()],
}) || helper.state
);
createSearchResults(helper.state);
const scopedResults = parentIndex
.getScopedResults()
.map((scopedResult) => {
Expand All @@ -124,7 +126,7 @@ export function useConnector<
instantSearchInstance: search,
results,
scopedResults,
state: results._state,
state: helper.state,
renderState: search.renderState,
templatesConfig: search.templatesConfig,
createURL: parentIndex.createURL,
Expand Down