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(index): support adding index widget with initial UI state #4359

Merged
merged 16 commits into from
May 13, 2020
Merged
4 changes: 3 additions & 1 deletion src/lib/InstantSearch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,9 @@ See ${createDocumentationLink({
}

public scheduleSearch = defer(() => {
this.mainHelper!.search();
if (this.started) {
this.mainHelper!.search();
}
});

public scheduleRender = defer(() => {
Expand Down
22 changes: 22 additions & 0 deletions src/lib/__tests__/escape-highlight-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -246,4 +246,26 @@ describe('escapeHits()', () => {

expect(hits).toEqual(output);
});

it('should not mutate the hit', () => {
const hit = {
_highlightResult: {
foobar: {
value: '<script>__ais-highlight__foo__/ais-highlight__</script>',
},
},
};

const hits = [hit];

escapeHits(hits);

expect(hit).toEqual({
_highlightResult: {
foobar: {
value: '<script>__ais-highlight__foo__/ais-highlight__</script>',
},
},
});
});
});
12 changes: 7 additions & 5 deletions src/lib/escape-highlight.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,15 @@ function recursiveEscape(input: any): any {

export default function escapeHits(hits: Hit[]): Hit[] {
if ((hits as any).__escaped === undefined) {
hits = hits.map(hit => {
if (hit._highlightResult) {
hit._highlightResult = recursiveEscape(hit._highlightResult);
// We don't override the value on hit because it will mutate the raw results
// instead we make a shallow copy and we assign the escaped values on it.
hits = hits.map(({ _highlightResult, _snippetResult, ...hit }) => {
if (_highlightResult) {
hit._highlightResult = recursiveEscape(_highlightResult);
}

if (hit._snippetResult) {
hit._snippetResult = recursiveEscape(hit._snippetResult);
if (_snippetResult) {
hit._snippetResult = recursiveEscape(_snippetResult);
}

return hit;
Expand Down
49 changes: 49 additions & 0 deletions src/widgets/index/__tests__/index-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ describe('index', () => {
}),
getWidgetState(uiState) {
return {
...uiState,
configure: {
...uiState.configure,
...params,
Expand Down Expand Up @@ -1783,6 +1784,54 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/index-widge
});
});
});

it('uiState on inner index does not get erased on addWidget', () => {
const level0 = index({ indexName: 'level0IndexName' });

const searchClient = createSearchClient();
const mainHelper = algoliasearchHelper(searchClient, '', {});
const instantSearchInstance = createInstantSearch({
mainHelper,
_initialUiState: {
level0IndexName: {
query: 'something',
},
},
});

instantSearchInstance.mainIndex.init(
createInitOptions({ instantSearchInstance, parent: null })
);

instantSearchInstance.mainIndex.addWidgets([level0]);

level0.init(
createInitOptions({
instantSearchInstance,
parent: instantSearchInstance.mainIndex,
})
);

level0.addWidgets([
createConfigure({
distinct: false,
}),
]);

level0.addWidgets([createSearchBox()]);

expect(level0.getHelper()!.state.query).toBe('something');

expect(instantSearchInstance.mainIndex.getWidgetState({})).toEqual({
indexName: {},
level0IndexName: {
configure: {
distinct: false,
},
query: 'something',
},
});
});
});

describe('render', () => {
Expand Down
80 changes: 66 additions & 14 deletions src/widgets/index/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import algoliasearchHelper, {
PlainSearchParameters,
SearchParameters,
SearchResults,
AlgoliaSearchHelper,
} from 'algoliasearch-helper';
import {
InstantSearch,
Expand Down Expand Up @@ -68,9 +69,38 @@ export function isIndexWidget(widget: Widget): widget is Index {
return widget.$$type === 'ais.index';
}

/**
* This is the same content as helper._change / setState, but allowing for extra
* _isFromAddWidget, which we use to decide whether local ui state invalidates.
*/
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
function privateHelperSetState(
helper: AlgoliaSearchHelper,
{
state,
isPageReset,
_isFromAddWidget,
Copy link
Member

Choose a reason for hiding this comment

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

(1) Instead of saying where it's from, what about saying what it does? (similarly to isPageReset). This name looses sense suddenly if we were to reuse it somewhere else. A suggestion: _shouldInheritUiState.

(2) To make it more testable and easier to think about, I would rather pass the baseUiState object here, instead of a boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, I've changed it to _uiState, where we pass the local ui state in this case

}: {
state: SearchParameters;
isPageReset?: boolean;
_isFromAddWidget?: boolean;
}
) {
if (state !== helper.state) {
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
helper.state = state;

helper.emit('change', {
state: helper.state,
results: helper.lastResults,
isPageReset,
_isFromAddWidget,
});
}
}

function getLocalWidgetsState(
widgets: Widget[],
widgetStateOptions: WidgetStateOptions
widgetStateOptions: WidgetStateOptions,
initialUiState: IndexUiState = {}
): IndexUiState {
return widgets
.filter(widget => !isIndexWidget(widget))
Expand All @@ -80,7 +110,7 @@ function getLocalWidgetsState(
}

return widget.getWidgetState(uiState, widgetStateOptions);
}, {});
}, initialUiState);
}

function getLocalWidgetsSearchParameters(
Expand Down Expand Up @@ -110,8 +140,11 @@ function resetPageFromWidgets(widgets: Widget[]): void {
indexWidgets.forEach(widget => {
const widgetHelper = widget.getHelper()!;

// @ts-ignore @TODO: remove "ts-ignore" once `resetPage()` is typed in the helper
widgetHelper.setState(widgetHelper.state.resetPage());
privateHelperSetState(widgetHelper, {
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
// @ts-ignore @TODO: remove "ts-ignore" once `resetPage()` is typed in the helper
state: widgetHelper.state.resetPage(),
isPageReset: true,
});

resetPageFromWidgets(widget.getWidgets());
});
Expand Down Expand Up @@ -213,19 +246,23 @@ const index = (props: IndexProps): Index => {
localWidgets = localWidgets.concat(widgets);

if (localInstantSearchInstance && Boolean(widgets.length)) {
helper!.setState(
getLocalWidgetsSearchParameters(localWidgets, {
privateHelperSetState(helper!, {
state: getLocalWidgetsSearchParameters(localWidgets, {
uiState: localUiState,
initialSearchParameters: helper!.state,
})
);
}),
_isFromAddWidget: true,
});

widgets.forEach(widget => {
if (localInstantSearchInstance && widget.init) {
widget.init({
helper: helper!,
parent: this,
uiState: {},
// Only index widget uses this key. This means that the initial value
// is sufficient, since that index didn't yet exist. If we see wrong
// behavior with this, use mainIndex.getWidgetState()
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
uiState: localInstantSearchInstance._initialUiState,
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
instantSearchInstance: localInstantSearchInstance,
state: helper!.state,
templatesConfig: localInstantSearchInstance.templatesConfig,
Expand Down Expand Up @@ -402,11 +439,26 @@ const index = (props: IndexProps): Index => {
// widgets. When the subscription happens before the `init` step, the (static)
// configuration of the widget is pushed in the URL. That's what we want to avoid.
// https://github.com/algolia/instantsearch.js/pull/994/commits/4a672ae3fd78809e213de0368549ef12e9dc9454
helper.on('change', ({ state }) => {
localUiState = getLocalWidgetsState(localWidgets, {
searchParameters: state,
helper: helper!,
});
helper.on('change', event => {
const { state, isPageReset } = event;

// @ts-ignore _isFromAddWidget comes from privateHelperSetState and
// thus isn't typed on the helper event
const _isFromAddWidget = event._isFromAddWidget;

if (isPageReset) {
localUiState.page = undefined;
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
}

localUiState = getLocalWidgetsState(
localWidgets,
{
searchParameters: state,
helper: helper!,
},
// @MAJOR in a next version we can always use localUiState, instead of usually empty object), but it requires every single widget to set an empty value (or remove the state key's value) if they are not refined.
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
_isFromAddWidget ? localUiState : {}
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
);

// We don't trigger an internal change when controlled because it
// becomes the responsibility of `setUiState`.
Expand Down
12 changes: 5 additions & 7 deletions test/mock/createInstantSearch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,17 @@ export const createInstantSearch = (
const mainHelper = algoliasearchHelper(searchClient, indexName, {});
const mainIndex = index({ indexName });

let started = false;

return {
indexName,
mainIndex,
mainHelper,
client: searchClient,
started,
start: () => {
started = true;
started: false,
start() {
this.started = true;
},
dispose: () => {
started = false;
dispose() {
this.started = false;
},
refresh: jest.fn(),
helper: mainHelper, // @TODO: use the Helper from the index once the RoutingManger uses the index
Expand Down