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

Commit

Permalink
fix(InstantSearch): update the searchState on props update (#2852)
Browse files Browse the repository at this point in the history
**Summary**

This PR fixes an issue where an infinite loop occurs with the controlled mode. The update of the `searchState` was done too late. With V5 the update was done inside `cWRP` but with V6 we move the update inside `cDU`. This change has an impact on when the `searchState` is updated. With `cWRP` it was executed before the `render` of the children, it's not the case anymore with `cDU`. It's done after.

Those children can read the `searchState` provided by the manager. When it's not up to date they alter the previous version of the state. With an `onSearchStateChange` function that relies on the value of `searchState` to add remove parameters, it can lead to infinite loop because the props are changes/reverted at each render.

The fix uses `getDerivedStateFromProps` to keep the `searchState` up to date. The issue with this solution (on V5 too) is that we might trigger one useless request when the props change. It's because the update and the search are tied together inside `onExternalStateUpdate`. One solution to this would be to decouple the two. Apply the `searchState` update as soon as possible but trigger the search on `cDU` once we've computed the state from the children.

**Before**

![before](https://user-images.githubusercontent.com/6513513/66131368-9c15d180-e5f3-11e9-9aad-c46ea6fca340.gif)

**After**

![after](https://user-images.githubusercontent.com/6513513/66131378-9f10c200-e5f3-11e9-8980-e6a6e5d626a4.gif)
  • Loading branch information
samouss committed Oct 11, 2019
1 parent 3cac5c4 commit c987087
Show file tree
Hide file tree
Showing 3 changed files with 149 additions and 85 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ describe('createInstantSearchManager', () => {
const {
mainParameters,
derivedParameters,
} = wrapper.instance().aisManager.getSearchParameters();
} = wrapper.instance().state.instantSearchManager.getSearchParameters();

expect(mainParameters).toEqual(
expect.objectContaining({
Expand Down Expand Up @@ -704,7 +704,7 @@ describe('createInstantSearchManager', () => {
const {
mainParameters,
derivedParameters,
} = wrapper.instance().aisManager.getSearchParameters();
} = wrapper.instance().state.instantSearchManager.getSearchParameters();

expect(mainParameters).toEqual(
expect.objectContaining({
Expand Down
95 changes: 55 additions & 40 deletions packages/react-instantsearch-core/src/widgets/InstantSearch.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, { Component, Children } from 'react';
import isEqual from 'fast-deep-equal';
import PropTypes from 'prop-types';
import createInstantSearchManager from '../core/createInstantSearchManager';
import { InstantSearchProvider, InstantSearchContext } from '../core/context';
Expand Down Expand Up @@ -56,6 +57,8 @@ type Props = {
};

type State = {
isControlled: boolean;
instantSearchManager: InstantSearchManager;
contextValue: InstantSearchContext;
};

Expand Down Expand Up @@ -136,7 +139,19 @@ class InstantSearch extends Component<Props, State> {
nextProps: Props,
prevState: State
): Partial<State> {
const nextIsControlled = isControlled(nextProps);
const previousSearchState = prevState.instantSearchManager.store.getState()
.widgets;
const nextSearchState = nextProps.searchState;

if (nextIsControlled && !isEqual(previousSearchState, nextSearchState)) {
prevState.instantSearchManager.onExternalStateUpdate(
nextProps.searchState
);
}

return {
isControlled: nextIsControlled,
contextValue: {
...prevState.contextValue,
mainTargetedIndex: nextProps.indexName,
Expand All @@ -145,83 +160,83 @@ class InstantSearch extends Component<Props, State> {
}

isUnmounting: boolean = false;
aisManager: InstantSearchManager = createInstantSearchManager({
indexName: this.props.indexName,
searchClient: this.props.searchClient,
initialState: this.props.searchState || {},
resultsState: this.props.resultsState,
stalledSearchDelay: this.props.stalledSearchDelay,
});
isControlled: boolean = isControlled(this.props);

state = {
contextValue: {

constructor(props: Props) {
super(props);

const instantSearchManager = createInstantSearchManager({
indexName: this.props.indexName,
searchClient: this.props.searchClient,
initialState: this.props.searchState || {},
resultsState: this.props.resultsState,
stalledSearchDelay: this.props.stalledSearchDelay,
});

const contextValue = {
store: instantSearchManager.store,
widgetsManager: instantSearchManager.widgetsManager,
mainTargetedIndex: this.props.indexName,
onInternalStateUpdate: this.onWidgetsInternalStateUpdate.bind(this),
createHrefForState: this.createHrefForState.bind(this),
onSearchForFacetValues: this.onSearchForFacetValues.bind(this),
onSearchStateChange: this.onSearchStateChange.bind(this),
onSearchParameters: this.onSearchParameters.bind(this),
store: this.aisManager.store,
widgetsManager: this.aisManager.widgetsManager,
mainTargetedIndex: this.props.indexName,
},
};
};

this.state = {
isControlled: isControlled(this.props),
instantSearchManager,
contextValue,
};
}

componentDidUpdate(prevProps: Props) {
const nextIsControlled = isControlled(this.props);
const prevIsControlled = isControlled(prevProps);

if (!this.isControlled && nextIsControlled) {
if (prevIsControlled && !this.state.isControlled) {
throw new Error(
"You can't switch <InstantSearch> from being uncontrolled to controlled"
"You can't switch <InstantSearch> from being controlled to uncontrolled"
);
}

if (this.isControlled && !nextIsControlled) {
if (!prevIsControlled && this.state.isControlled) {
throw new Error(
"You can't switch <InstantSearch> from being controlled to uncontrolled"
"You can't switch <InstantSearch> from being uncontrolled to controlled"
);
}

/*
* Clear cache when `refresh` changes to `true`.
* Prevents users to always clear the cache on render if they forget to revert it to `false`.
*/
if (this.props.refresh !== prevProps.refresh && this.props.refresh) {
this.aisManager.clearCache();
}

if (this.isControlled) {
this.aisManager.onExternalStateUpdate(this.props.searchState);
this.state.instantSearchManager.clearCache();
}

if (prevProps.indexName !== this.props.indexName) {
this.aisManager.updateIndex(this.props.indexName);
this.state.instantSearchManager.updateIndex(this.props.indexName);
}

if (prevProps.searchClient !== this.props.searchClient) {
this.aisManager.updateClient(this.props.searchClient);
this.state.instantSearchManager.updateClient(this.props.searchClient);
}
}

componentWillUnmount() {
this.isUnmounting = true;
this.aisManager.skipSearch();
this.state.instantSearchManager.skipSearch();
}

createHrefForState(searchState: SearchState) {
searchState = this.aisManager.transitionState(searchState);
return this.isControlled && this.props.createURL
searchState = this.state.instantSearchManager.transitionState(searchState);
return this.state.isControlled && this.props.createURL
? this.props.createURL(searchState, this.getKnownKeys())
: '#';
}

onWidgetsInternalStateUpdate(searchState: SearchState) {
searchState = this.aisManager.transitionState(searchState);
searchState = this.state.instantSearchManager.transitionState(searchState);

this.onSearchStateChange(searchState);

if (!this.isControlled) {
this.aisManager.onExternalStateUpdate(searchState);
if (!this.state.isControlled) {
this.state.instantSearchManager.onExternalStateUpdate(searchState);
}
}

Expand All @@ -244,11 +259,11 @@ class InstantSearch extends Component<Props, State> {
}

onSearchForFacetValues(searchState) {
this.aisManager.onSearchForFacetValues(searchState);
this.state.instantSearchManager.onSearchForFacetValues(searchState);
}

getKnownKeys() {
return this.aisManager.getWidgetsIds();
return this.state.instantSearchManager.getWidgetsIds();
}

render() {
Expand Down
Loading

0 comments on commit c987087

Please sign in to comment.