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

Conversation

francoischalifour
Copy link
Member

Description

This changes how we compute the initial Helper state that we give to widget.getWidgetRenderState() to retrieve the initial widget React state.

The problem originally surfaced when server-side rendering the <Menu> component:

Error: Cannot refine the undeclared facet categories; it should be added to the helper options facets, disjunctiveFacets or hierarchicalFacets

This happens because the helper.state and state used to call widget.getWidgetRenderState() mismatched. In algolia/instantsearch#4960, we tried to fix this problem by always relying on state. But this constraint is hard to respect, and hard to implement in practice because of stale closures (in refine() and createURL()).

I explain in that PR why we had to do it this way, essentially because the helper state was the resolved helper state, including all the inherited search parameters from the parent indices. With a resolved helper state, InstantSearch considers the parameters as controlled and therefore doesn't get notified of new changes coming from other indices. The part that we were missing is that there's a way to compute the local index search parameters without inheriting the parent index via index.getWidgetSearchParameters().

Solution

This PR is another cleaner approach directly in React InstantSearch Hooks. We assign the widget search parameters gotten from widget.getWidgetSearchParameters() to the helper state so that the widget is directly aware of its search parameters.

Result

  • It fixes the original problem with <Menu> throwing about the undeclared facet.
  • It also fixes useClearRefinements() and useCurrentRefinements() that used to initially render with an empty state. They're now directly aware of their refinements during first render.
  • We'll be able to rollback some of the changes introduced in feat: rely on state in getWidgetRenderState instantsearch#4960

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 51b0820:

Sandbox Source
react-instantsearch-app Configuration
routing-basic Configuration
hooks-example Configuration

@netlify
Copy link

netlify bot commented Mar 24, 2022

Deploy Preview for react-instantsearch ready!

Name Link
🔨 Latest commit 51b0820
🔍 Latest deploy log https://app.netlify.com/sites/react-instantsearch/deploys/623c9e9e87f3d200081e2743
😎 Deploy Preview https://deploy-preview-3399--react-instantsearch.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

I'll approve this, because I'm not seeing how what I described can cause real issues.

Maybe we should call widget.dispose after the initial getWidgetRenderState and before it "really gets mounted"? Or some other way to set helper.state back to the previous value after fake rendering (cache state; getWidgetRenderState; reset state;)?

Comment on lines +95 to +97
helper.state =
widget.getWidgetSearchParameters?.(helper.state, { uiState }) ||
helper.state;
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.

@francoischalifour francoischalifour merged commit b4bd933 into master Mar 28, 2022
@francoischalifour francoischalifour deleted the fix/useConnector-helper-state branch March 28, 2022 15:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants