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

feat: rely on state in getWidgetRenderState #4960

Merged
merged 1 commit into from
Nov 26, 2021

Conversation

francoischalifour
Copy link
Member

Description

This changes how we provide the widgets' render state from the helper state to the retrieved state (renderOptions.helper.state vs. renderOptions.state).

This is required to support React InstantSearch Hooks SSR1 to avoid flashes between SSR and CSR2 when hydrating.

Explanation

As opposed to Vue InstantSearch SSR, React InstantSearch Hooks SSR works in a single data flow pass. We rely on a single helper (the one created by InstantSearch.js), to provide the search state and the search results to the widgets.

By design, InstantSearch.js computes the search parameters from the mounted widgets (e.g., searchBox controls the query search parameter). As soon as you start setting a search parameter that is not yet controlled by a widget (this happens when a child index inherits from its parent index searchBox state), InstantSearch isn't able to take control back on that search parameter. This is for this reason that we cannot override the helper state (aka search parameters) before widgets are mounted, and then rely on the widgets to update the helper state again.

Because of this constraint, once we override the initial helper state on SSR, any interactions on the browser won't update the search parameters, leaving the UI stuck.

This PR modifies the way we return the widgets' render state from getWidgetRenderState to rely directly on state, and not helper.state. This lets us provide a computed initial state from results._state without controlling the search parameters yet, and display the correct UI state on screen without messing with InstantSearch internals. And therefore lets InstantSearch correctly controls the search parameters after user interactions.

Impact

This change is only internal and shouldn't impact any user implementations.

Footnotes

  1. SSR: Server-Side Rendering

  2. CSR: Client-Side Rendering

@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 f3a5a73:

Sandbox Source
InstantSearch.js Configuration

@@ -303,8 +303,8 @@ const connectInfiniteHits: InfiniteHitsConnector = function connectInfiniteHits(
widgetType: this.$$type,
});
isFirstPage =
helper.state.page === undefined ||
Copy link
Contributor

@Haroenv Haroenv Nov 24, 2021

Choose a reason for hiding this comment

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

I wonder if in the index widget, could we make the getter of helper.state warn? Or is that still used internally on eg. setState?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are still valid use cases for using helper.state in callbacks like createURL() or refine() to manipulate the latest helper state:

https://github.com/algolia/instantsearch.js/blob/7747c50c037b1a73cf74e9e82986a0c303822e5a/src/connectors/breadcrumb/connectBreadcrumb.ts#L204-L206

An ESLint rule should be able to catch that maybe? Not really trivial though. Ultimately we want the values from the main paths that are returned from getWidgetRenderState to be computed with state, not with helper.state, the rest is fine.

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.

this change lgtm, as even if we fix this issue in React InstantSearch server side rendering later, state should be consistent with helper.state.

In a future version I'd like to see state being the same as helper.state though, and maybe even only one of the two exposed

@francoischalifour francoischalifour merged commit 5006841 into master Nov 26, 2021
@francoischalifour francoischalifour deleted the fix/render-state-based-on-helper-state branch November 26, 2021 10:45
if (query !== helper.state.query) {
if (query !== state.query) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is what caused the bug (state is stale in this callback), but the part of code isn't actually needed, we can still set the query and search without condition. Fixing in #4990

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants