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

Haroenv · 2023-01-12T13:07:28Z

Summary

This change enables optimistic rendering, as discussed in RFC 82 and also #4403.

This doesn't (yet) implement reverting the state on error, as it is done in #5438.

Result

when the search/loading starts, a render happens as well
when we render, we use the current SearchParameters for _state in SearchResults
fix all edge cases that didn't correctly detect idle state before persisting state
introduce common unit/integration tests for optimistic ui in RefinementList, Pagination, Menu, HierarchicalMenu

FX-2196

codesandbox-ci · 2023-01-12T13:10:54Z

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 a15a451:

Sandbox	Source
InstantSearch.js	Configuration
react-instantsearch-app	Configuration
example-react-instantsearch-hooks-default-theme	Configuration
example-vue-instantsearch-default-theme	Configuration

…re about isRefined)

this is because of the change in render-args, but i don't see how

examples/react-hooks/default-theme/src/App.tsx

packages/instantsearch.js/src/lib/__tests__/status.test.ts

packages/instantsearch.js/src/lib/utils/createSendEventForHits.ts

packages/instantsearch.js/src/lib/utils/render-args.ts

Haroenv · 2023-01-12T18:08:27Z

packages/instantsearch.js/src/lib/InstantSearch.ts

-      // @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);


this is part B of the implementation

packages/instantsearch.js/src/widgets/infinite-hits/__tests__/infinite-hits-integration-test.ts

Haroenv · 2023-01-12T18:15:39Z

...es/react-instantsearch-hooks-server/src/__tests__/__snapshots__/getServerState.test.tsx.snap

-      "disjunctiveFacets": [
-        "brand",
-      ],
-      "disjunctiveFacetsRefinements": {
-        "brand": [
-          "Apple",
-        ],
-      },
+      "disjunctiveFacets": [],
+      "disjunctiveFacetsRefinements": {},


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

packages/instantsearch.js/src/lib/utils/render-args.ts

seems that react may skip the render if `loading` is visible <1 frame

packages/instantsearch.js/src/lib/utils/render-args.ts

Haroenv · 2023-01-16T12:48:29Z

packages/instantsearch.js/src/widgets/index/index.ts

    getResults() {
-      return derivedHelper && derivedHelper.lastResults;
+      if (!derivedHelper) return null;
+      if (!derivedHelper.lastResults) return null;
+
+      // 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.
+      derivedHelper.lastResults._state = helper!.state;
+
+      return derivedHelper.lastResults;
    },


this is part A of the implementation

Haroenv · 2023-01-18T10:15:14Z

packages/react-instantsearch-hooks/src/components/__tests__/InstantSearchSSRProvider.test.tsx

+    expect(searchClient.search).toHaveBeenCalledTimes(0);
+    expect(appleRefinement).not.toBeChecked();
+
    userEvent.click(appleRefinement);

    await waitFor(() => {
-      expect(appleRefinement).toBeChecked();
+      expect(searchClient.search).toHaveBeenCalledTimes(1);
+      // possible bug in testing-library, this is checked in real life in the same situation
+      // expect(appleRefinement).toBeChecked();


This test was never correctly passing:

searchClient wasn't correct (returned no facets on result)

and behaviour it pretended to fix was never fixed (if result._state said something was refined, it would show as refined, but first click would only set it to refined again, second click would toggle again, because local search parameters are used for refine)

And the behaviour changed in this PR. It's sufficiently covered under the expanded "renders refinements from local widget state" test now.

dhayab

👏

packages/instantsearch.js/src/widgets/index/index.ts

Co-authored-by: Dhaya <154633+dhayab@users.noreply.github.com>

sarahdayan

In addition to your existing tests, I think each flavor should have the same suite of user-oriented tests (testing the UI, not checking what methods are called) where we slow down the search request, click a refinement, and assess whether the checkbox is checked or not (before this PR it should not, with this PR it should).

Ideally, we do this for every impacted widget (all refinement widgets, pagination, etc.) This could be in a optimistic-ui test suite.

Later on, when we do a common test suite as in Predict, we could factorize that into one.

packages/instantsearch.js/src/connectors/infinite-hits/connectInfiniteHits.ts

packages/instantsearch.js/src/lib/__tests__/InstantSearch-test.tsx

packages/instantsearch.js/src/widgets/index/index.ts

packages/react-instantsearch-hooks/src/hooks/__tests__/useInstantSearch.test.tsx

packages/vue-instantsearch/src/util/__tests__/createServerRootMixin.test.js

packages/instantsearch.js/src/lib/utils/createSendEventForHits.ts

Not used finally in the test (because timing still is complicated), but it would be possible later

Haroenv · 2023-01-20T10:53:26Z

packages/instantsearch.js/src/components/Pagination/Pagination.tsx

-            ariaLabel={`${pageNumber + 1}`}
+            ariaLabel={`Page ${pageNumber + 1}`}


other aria-labels aren't configurable, so changing this one to add text isn't a breaking change

Can those changes be easily moved into their own PR? It adds a bit of noise and this one is already quite large.

The tests would fail or need to be skipped between the tests. Happy to revert in this PR if needed

Ah that's right, I saw you use those new aria label strings in tests. It's fine for me.

Yes, it's added because they all three used different aria-label before this PR, so I didn't have a way to target the right items

Haroenv · 2023-01-20T10:53:45Z

packages/vue-instantsearch/src/components/Pagination.vue

@@ -88,6 +88,7 @@
            <a
              :class="suit('link')"
              :href="state.createURL(page)"
+              :aria-label="`Page ${page + 1}`"


other aria-labels aren't configurable, so adding this one to add text isn't a breaking change

Haroenv · 2023-01-20T10:55:28Z

packages/react-instantsearch-hooks/src/components/__tests__/InstantSearchSSRProvider.test.tsx

-  // Fixes https://github.com/algolia/react-instantsearch/issues/3530
-  test('renders initial refinement and allows to refine them', async () => {


mentioned in a different comment too, but this test wasn't actually asserting what it was meant to (toBeChecked at the bottom should be .not.toBeChecked if we actually fixed the issue, but we never did). The similar assertion about it being interactive is added to the other test earlier.

Haroenv · 2023-01-20T10:56:26Z

packages/instantsearch.js/src/widgets/index/index.ts

+      // To make the UI optimistic, we patch the state to display to the current
+      // one instead of the one associated with the latest results.
+      // This means user-driven UI changes (e.g., checked checkbox) are reflected
+      // immediately instead of waiting for Algolia to respond, regardless of
+      // the status of the network request.
+      derivedHelper.lastResults._state = helper!.state;


root of the implementation

dhayab

Great set up for the common test suite!

dhayab · 2023-01-20T12:47:52Z

packages/instantsearch.js/src/components/Pagination/Pagination.tsx

-            ariaLabel={`${pageNumber + 1}`}
+            ariaLabel={`Page ${pageNumber + 1}`}


Can those changes be easily moved into their own PR? It adds a bit of noise and this one is already quite large.

tests/common-unit/hierarchical-menu/optimistic-ui.ts

dhayab · 2023-01-20T13:00:14Z

tests/common-unit/hierarchical-menu/optimistic-ui.ts

+      // wait for initial results
+      await act(async () => {
+        await wait(margin + delay);
+        await wait(0);


Could you explain or comment why we wait a second time after most of the interactions?

I don't fully understand, but I believe it's related to jest timers being quite strict in ordering. If there's no two timers waited for, a timer added during the initial wait will still be triggered after.

sarahdayan

My comments for the hierarchical-menu common suite apply to others when relevant.

tests/common-unit/hierarchical-menu/index.ts

tests/common-unit/hierarchical-menu/optimistic-ui.ts

tests/common-unit/package.json

aymeric-giraudet

Nice PR ! Great job on abstracting the common test suites :D

tests/common/widgets/hierarchical-menu/optimistic-ui.ts

also enables those relevant tests

helper update caused a tiny bit increase, and React InstantSearch connectors was sized tightly

sarahdayan

Gorgeous work on this topic, and I'm excited we have a common test suite now. Can't wait to move most our tests there!

tests/common/widgets/hierarchical-menu/optimistic-ui.ts

FabienMotte

Really nice work on the common test suite 👏

WIP

169aedf

Haroenv added 3 commits January 12, 2023 16:17

use only _state, as you can otherwise get into edge cases (we only ca…

7998921

…re about isRefined)

take extra render in account

f403bc2

WEIRD: getServerState no longer inherits parameters?

9254365

this is because of the change in render-args, but i don't see how

Haroenv commented Jan 12, 2023

View reviewed changes

Haroenv added 2 commits January 12, 2023 19:24

slow down flaky test

d1da45c

seems that react may skip the render if `loading` is visible <1 frame

remove stray part

e9ef960

Haroenv commented Jan 12, 2023

View reviewed changes

packages/instantsearch.js/src/lib/utils/render-args.ts Outdated Show resolved Hide resolved

Haroenv added 2 commits January 16, 2023 13:46

revert example

5fffc6d

move patch

4b142a4

Haroenv commented Jan 16, 2023

View reviewed changes

Haroenv changed the title ~~WIP: optimistic rendering~~ feat(rendering): always render with current state Jan 17, 2023

test(unit): make tests pass taking new source of state in account

0833320

Haroenv force-pushed the feat/optimistic-rendering branch from b0e8d6a to 0833320 Compare January 17, 2023 18:44

Haroenv commented Jan 17, 2023

View reviewed changes

remove wrong test, assert in existing one

3951d02

Haroenv marked this pull request as ready for review January 18, 2023 10:37

Haroenv requested review from sarahdayan, aymeric-giraudet, dhayab and FabienMotte January 18, 2023 11:00

dhayab approved these changes Jan 18, 2023

View reviewed changes

packages/instantsearch.js/src/widgets/index/index.ts Outdated Show resolved Hide resolved

Update packages/instantsearch.js/src/widgets/index/index.ts

bc85527

Co-authored-by: Dhaya <154633+dhayab@users.noreply.github.com>

sarahdayan reviewed Jan 18, 2023

View reviewed changes

Haroenv mentioned this pull request Jan 18, 2023

feat(rendering): revert search state on error #5438

Merged

Haroenv added 4 commits January 19, 2023 09:42

chore: rephrase optimistic comment

5efe66f

chore(controlledClient): make more usable

4896e3d

Not used finally in the test (because timing still is complicated), but it would be possible later

chore(tests): expose mocks from index

8485397

test(unit): set up common tests for optimistic UI

ad65263

Haroenv added 3 commits January 20, 2023 11:44

chore(test): clear mocks between tests

bf6a14c

test: update snapshots

0c7e0eb

skip hierarchical tests correctly

18e4b87

Haroenv requested review from dhayab and sarahdayan January 20, 2023 10:47

more mockReset

de4e2de

Haroenv commented Jan 20, 2023

View reviewed changes

Haroenv added 4 commits January 20, 2023 12:38

fix missed number

f693561

vue compat with v3

b623c89

simplify element in body

a86450a

use vm again (it fails in vue 2)

6b600a2

dhayab reviewed Jan 20, 2023

View reviewed changes

sarahdayan reviewed Jan 20, 2023

View reviewed changes

Haroenv added 4 commits January 20, 2023 15:42

move tests

681439c

tests(common): move to widgetParams per test

3161af8

update comments

8d7bc8c

test: move to testing-lib for accessible selectors

db57076

Haroenv requested a review from sarahdayan January 20, 2023 15:45

test: get rid of container + return of setup

fc51cbf

aymeric-giraudet approved these changes Jan 20, 2023

View reviewed changes

tests/common/widgets/hierarchical-menu/optimistic-ui.ts Show resolved Hide resolved

Haroenv added 3 commits January 23, 2023 09:44

test(common): clean before starting

552de07

fix(helper): update to ensure _state is read for hierarchical

89c38a0

also enables those relevant tests

chore(size): update bundlesize

67de404

helper update caused a tiny bit increase, and React InstantSearch connectors was sized tightly

sarahdayan approved these changes Jan 23, 2023

View reviewed changes

tests/common/widgets/hierarchical-menu/optimistic-ui.ts Outdated Show resolved Hide resolved

reword comment

a15a451

Haroenv enabled auto-merge (squash) January 23, 2023 15:46

FabienMotte approved these changes Jan 23, 2023

View reviewed changes

Haroenv merged commit 920e951 into master Jan 23, 2023

Haroenv deleted the feat/optimistic-rendering branch January 23, 2023 16:11

Haroenv mentioned this pull request Aug 9, 2024

RefinementList is getting checked after the results are refined. Bad UX for slow internet connections #5268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Haroenv commented Jan 12, 2023 •

edited

Loading

codesandbox-ci bot commented Jan 12, 2023 •

edited

Loading

Haroenv Jan 12, 2023

Haroenv Jan 12, 2023 •

edited

Loading

Haroenv Jan 16, 2023

This comment was marked as outdated.

Haroenv Jan 18, 2023 •

edited

Loading

dhayab left a comment

sarahdayan left a comment

Haroenv Jan 20, 2023

dhayab Jan 20, 2023

Haroenv Jan 20, 2023

dhayab Jan 20, 2023

Haroenv Jan 20, 2023

Haroenv Jan 20, 2023

Haroenv Jan 20, 2023

Haroenv Jan 20, 2023

dhayab left a comment

dhayab Jan 20, 2023

dhayab Jan 20, 2023

Haroenv Jan 20, 2023

sarahdayan left a comment

aymeric-giraudet left a comment

sarahdayan left a comment

FabienMotte left a comment

		ariaLabel={`${pageNumber + 1}`}
		ariaLabel={`Page ${pageNumber + 1}`}

		// Fixes https://github.com/algolia/react-instantsearch/issues/3530
		test('renders initial refinement and allows to refine them', async () => {

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

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

Conversation

Haroenv commented Jan 12, 2023 • edited Loading

codesandbox-ci bot commented Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Haroenv Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Haroenv Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

dhayab left a comment

Choose a reason for hiding this comment

sarahdayan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhayab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahdayan left a comment

Choose a reason for hiding this comment

aymeric-giraudet left a comment

Choose a reason for hiding this comment

sarahdayan left a comment

Choose a reason for hiding this comment

FabienMotte left a comment

Choose a reason for hiding this comment

Haroenv commented Jan 12, 2023 •

edited

Loading

codesandbox-ci bot commented Jan 12, 2023 •

edited

Loading

Haroenv Jan 12, 2023 •

edited

Loading

Haroenv Jan 18, 2023 •

edited

Loading