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.

Sign up for GitHub

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(rendering): always render with current state #5429

Merged
merged 33 commits into from
Jan 23, 2023
Merged

feat(rendering): always render with current state #5429

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
169aedf
WIP
Haroenv Jan 12, 2023
7998921
use only _state, as you can otherwise get into edge cases (we only ca…
Haroenv Jan 12, 2023
f403bc2
take extra render in account
Haroenv Jan 12, 2023
9254365
WEIRD: getServerState no longer inherits parameters?
Haroenv Jan 12, 2023
d1da45c
slow down flaky test
Haroenv Jan 12, 2023
e9ef960
remove stray part
Haroenv Jan 12, 2023
5fffc6d
revert example
Haroenv Jan 16, 2023
4b142a4
move patch
Haroenv Jan 16, 2023
0833320
test(unit): make tests pass taking new source of state in account
Haroenv Jan 17, 2023
3951d02
remove wrong test, assert in existing one
Haroenv Jan 18, 2023
bc85527
Update packages/instantsearch.js/src/widgets/index/index.ts
Haroenv Jan 18, 2023
5efe66f
chore: rephrase optimistic comment
Haroenv Jan 19, 2023
4896e3d
chore(controlledClient): make more usable
Haroenv Jan 20, 2023
8485397
chore(tests): expose mocks from index
Haroenv Jan 20, 2023
ad65263
test(unit): set up common tests for optimistic UI
Haroenv Jan 20, 2023
6a8ab08
fix(a11y): give a more complete aria-label to pagination
Haroenv Jan 20, 2023
bf6a14c
chore(test): clear mocks between tests
Haroenv Jan 20, 2023
0c7e0eb
test: update snapshots
Haroenv Jan 20, 2023
18e4b87
skip hierarchical tests correctly
Haroenv Jan 20, 2023
de4e2de
more mockReset
Haroenv Jan 20, 2023
f693561
fix missed number
Haroenv Jan 20, 2023
b623c89
vue compat with v3
Haroenv Jan 20, 2023
a86450a
simplify element in body
Haroenv Jan 20, 2023
6b600a2
use vm again (it fails in vue 2)
Haroenv Jan 20, 2023
681439c
move tests
Haroenv Jan 20, 2023
3161af8
tests(common): move to widgetParams per test
Haroenv Jan 20, 2023
8d7bc8c
update comments
Haroenv Jan 20, 2023
db57076
test: move to testing-lib for accessible selectors
Haroenv Jan 20, 2023
fc51cbf
test: get rid of container + return of setup
Haroenv Jan 20, 2023
552de07
test(common): clean before starting
Haroenv Jan 23, 2023
89c38a0
fix(helper): update to ensure _state is read for hierarchical
Haroenv Jan 23, 2023
67de404
chore(size): update bundlesize
Haroenv Jan 23, 2023
a15a451
reword comment
Haroenv Jan 23, 2023
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
6 changes: 5 additions & 1 deletion packages/instantsearch.js/src/connectors/infinite-hits/connectInfiniteHits.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,11 @@ const connectInfiniteHits: InfiniteHitsConnector = function connectInfiniteHits(
{ results }
);

if (cachedHits[page] === undefined && !results.__isArtificial) {
if (
cachedHits[page] === undefined &&
!results.__isArtificial &&
instantSearchInstance.status === 'idle'
) {
sarahdayan marked this conversation as resolved.
Show resolved Hide resolved
cachedHits[page] = transformedHits;
cache.write({ state, hits: cachedHits });
}
Expand Down
9 changes: 2 additions & 7 deletions packages/instantsearch.js/src/lib/InstantSearch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,12 +481,7 @@ See ${createDocumentationLink({

mainHelper.search = () => {
this.status = 'loading';
// @MAJOR: use scheduleRender here
// For now, widgets don't expect to be rendered at the start of `loading`,
// so it would be a breaking change to add an extra render. We don't have
// these guarantees about the render event, thus emitting it once more
// isn't a breaking change.
this.emit('render');
this.scheduleRender(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is part B of the implementation


// This solution allows us to keep the exact same API for the users but
// under the hood, we have a different implementation. It should be
Expand Down Expand Up @@ -639,7 +634,7 @@ See ${createDocumentationLink({
});

public scheduleRender = defer((shouldResetStatus: boolean = true) => {
if (!this.mainHelper!.hasPendingRequests()) {
if (!this.mainHelper?.hasPendingRequests()) {
clearTimeout(this._searchStalledTimer);
this._searchStalledTimer = null;

Expand Down
30 changes: 26 additions & 4 deletions packages/instantsearch.js/src/lib/__tests__/InstantSearch-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1324,10 +1324,14 @@ describe('scheduleStalledRender', () => {
// Trigger a new search
search.mainHelper!.search();

// search starts
await wait(0);
expect(widget.render).toHaveBeenCalledTimes(2);

// Reaches the delay
await wait(search._stalledSearchDelay);

expect(widget.render).toHaveBeenCalledTimes(2);
expect(widget.render).toHaveBeenCalledTimes(3);
});

it('deduplicates the calls to the `render` method', async () => {
Expand Down Expand Up @@ -1359,10 +1363,15 @@ describe('scheduleStalledRender', () => {
search.mainHelper!.search();
search.mainHelper!.search();

await wait(0);

// search starts
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
expect(widget.render).toHaveBeenCalledTimes(2);

// Reaches the delay
await wait(search._stalledSearchDelay);

expect(widget.render).toHaveBeenCalledTimes(2);
expect(widget.render).toHaveBeenCalledTimes(3);
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
});

it('triggers a `render` once the search expires the delay', async () => {
Expand Down Expand Up @@ -1405,11 +1414,24 @@ describe('scheduleStalledRender', () => {

expect(widget.render).toHaveBeenCalledTimes(1);

await wait(0);

// Widgets render because of the search
expect(widget.render).toHaveBeenCalledTimes(2);
expect(widget.render).toHaveBeenLastCalledWith(
expect.objectContaining({
searchMetadata: {
isSearchStalled: false,
},
status: 'loading',
})
);

// The delay is reached
await wait(search._stalledSearchDelay);

// Widgets render because of the stalled search
expect(widget.render).toHaveBeenCalledTimes(2);
expect(widget.render).toHaveBeenCalledTimes(3);
expect(widget.render).toHaveBeenLastCalledWith(
expect.objectContaining({
searchMetadata: {
Expand All @@ -1426,7 +1448,7 @@ describe('scheduleStalledRender', () => {
await wait(0);

// Widgets render because of the results
expect(widget.render).toHaveBeenCalledTimes(3);
expect(widget.render).toHaveBeenCalledTimes(4);
expect(widget.render).toHaveBeenLastCalledWith(
expect.objectContaining({
searchMetadata: {
Expand Down
1 change: 0 additions & 1 deletion packages/instantsearch.js/src/lib/__tests__/status.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,6 @@ describe('status', () => {
});

test('lets users render on error with the `render` event', async () => {
// expect.assertions(4);
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
const search = instantsearch({
indexName: 'indexName',
searchClient: createSearchClient({
Expand Down
10 changes: 5 additions & 5 deletions packages/instantsearch.js/src/lib/utils/createSendEventForHits.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@ const buildPayloads = ({
widgetType,
methodName,
args,
isSearchStalled,
invalidStatus,
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
}: {
widgetType: string;
index: string;
methodName: 'sendEvent' | 'bindEvent';
args: any[];
isSearchStalled: boolean;
invalidStatus: boolean;
}): InsightsEvent[] => {
// when there's only one argument, that means it's custom
if (args.length === 1 && typeof args[0] === 'object') {
Expand Down Expand Up @@ -87,7 +87,7 @@ const buildPayloads = ({
);

if (eventType === 'view') {
if (isSearchStalled) {
if (invalidStatus) {
return [];
}
return hitsChunks.map((batch, i) => {
Expand Down Expand Up @@ -163,7 +163,7 @@ export function createSendEventForHits({
index,
methodName: 'sendEvent',
args,
isSearchStalled: instantSearchInstance.status === 'stalled',
invalidStatus: instantSearchInstance.status !== 'idle',
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
});

payloads.forEach((payload) =>
Expand All @@ -186,7 +186,7 @@ export function createBindEventForHits({
index,
methodName: 'bindEvent',
args,
isSearchStalled: false,
invalidStatus: false,
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
});

return payloads.length
Expand Down
1 change: 1 addition & 0 deletions packages/instantsearch.js/src/widgets/index/__tests__/index-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2927,6 +2927,7 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/index-widge
},
],
_state: {
index: 'indexName',
disjunctiveFacets: [],
disjunctiveFacetsRefinements: {},
facets: [],
Expand Down
10 changes: 9 additions & 1 deletion packages/instantsearch.js/src/widgets/index/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,15 @@ const index = (widgetParams: IndexWidgetParams): IndexWidget => {
},

getResults() {
return derivedHelper && derivedHelper.lastResults;
if (!derivedHelper) return null;
if (!derivedHelper.lastResults) return null;
Haroenv marked this conversation as resolved.
Show resolved Hide resolved

// To make the UI optimistic, we will always render using the current state,
// but the previous results. This means a change will be visible immediately,
// regardless of the status of the network request.
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
derivedHelper.lastResults._state = helper!.state;

return derivedHelper.lastResults;
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is part A of the implementation


getScopedResults() {
Expand Down
54 changes: 27 additions & 27 deletions ...es/instantsearch.js/src/widgets/infinite-hits/__tests__/infinite-hits-integration-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,33 +144,33 @@ describe('infiniteHits', () => {
});
expect(customCache.write).toHaveBeenCalledTimes(2);
expect(customCache.write.mock.calls[1][0].hits).toMatchInlineSnapshot(`
{
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
"0": [
{
"__position": 1,
"objectID": "object-id0",
"title": "title 1",
},
{
"__position": 2,
"objectID": "object-id1",
"title": "title 2",
},
],
"1": [
{
"__position": 3,
"objectID": "object-id0",
"title": "title 3",
},
{
"__position": 4,
"objectID": "object-id1",
"title": "title 4",
},
],
}
`);
{
"0": [
{
"__position": 1,
"objectID": "object-id0",
"title": "title 1",
},
{
"__position": 2,
"objectID": "object-id1",
"title": "title 2",
},
],
"1": [
{
"__position": 3,
"objectID": "object-id0",
"title": "title 3",
},
{
"__position": 4,
"objectID": "object-id1",
"title": "title 4",
},
],
}
`);
});

it('displays all the hits from cache', async () => {
Expand Down
36 changes: 6 additions & 30 deletions ...react-instantsearch-hooks-server/src/__tests__/__snapshots__/getServerState.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,14 +200,8 @@ exports[`getServerState returns initialResults 1`] = `
},
],
"state": {
"disjunctiveFacets": [
"brand",
],
"disjunctiveFacetsRefinements": {
"brand": [
"Apple",
],
},
"disjunctiveFacets": [],
"disjunctiveFacetsRefinements": {},
Comment on lines -203 to +204
Copy link
Contributor Author

@Haroenv Haroenv Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We are now always rendering with the current state. The state from the results is wrong (merged) and thus useless. This PR changes that.

"facets": [],
"facetsExcludes": {},
"facetsRefinements": {},
Expand All @@ -216,9 +210,7 @@ exports[`getServerState returns initialResults 1`] = `
"highlightPostTag": "__/ais-highlight__",
"highlightPreTag": "__ais-highlight__",
"index": "instant_search_price_asc",
"maxValuesPerFacet": 10,
"numericRefinements": {},
"query": "iphone",
"tagRefinements": [],
},
},
Expand Down Expand Up @@ -310,14 +302,8 @@ exports[`getServerState returns initialResults 1`] = `
},
],
"state": {
"disjunctiveFacets": [
"brand",
],
"disjunctiveFacetsRefinements": {
"brand": [
"Apple",
],
},
"disjunctiveFacets": [],
"disjunctiveFacetsRefinements": {},
"facets": [],
"facetsExcludes": {},
"facetsRefinements": {},
Expand All @@ -326,9 +312,7 @@ exports[`getServerState returns initialResults 1`] = `
"highlightPostTag": "__/ais-highlight__",
"highlightPreTag": "__ais-highlight__",
"index": "instant_search_price_desc",
"maxValuesPerFacet": 10,
"numericRefinements": {},
"query": "iphone",
"tagRefinements": [],
},
},
Expand Down Expand Up @@ -420,14 +404,8 @@ exports[`getServerState returns initialResults 1`] = `
},
],
"state": {
"disjunctiveFacets": [
"brand",
],
"disjunctiveFacetsRefinements": {
"brand": [
"Apple",
],
},
"disjunctiveFacets": [],
"disjunctiveFacetsRefinements": {},
"facets": [],
"facetsExcludes": {},
"facetsRefinements": {},
Expand All @@ -436,9 +414,7 @@ exports[`getServerState returns initialResults 1`] = `
"highlightPostTag": "__/ais-highlight__",
"highlightPreTag": "__ais-highlight__",
"index": "instant_search_rating_desc",
"maxValuesPerFacet": 10,
"numericRefinements": {},
"query": "iphone",
"tagRefinements": [],
},
},
Expand Down
Loading