feat: preserve shared state on unmount with a future flag #5123

Haroenv · 2022-09-29T14:35:36Z

This PR introduces a functional change in how UI state is handled when widgets are removed from InstantSearch.

Currently, when a widget is removed, its dispose() method returns a cleaned state that is used to compute the new shared UI state. This means that if there are multiple widgets targeting the same attribute, when one of them is removed, the widget that is left has its UI state reset as a side-effect.

Through the introduction of future flags in InstantSearch, we are able to explicitly switch to a new dispose mode, which doesn't use the cleaned state returned by removed widgets, to allow multiple instances of widgets to keep the refinements active.

It's easy to opt into this new dispose method:

instantsearch({
  // ...
  future: {
    preserveSharedStateOnUnmount: true,
  },
});

Having this option enabled with a future flag allows us to easily release it without breaking existing implementations, allowing users to adopt it at their own rhythm.

This is made available for InstantSearch.js, React InstantSearch and Vue InstantSearch.

FX-2621

Initial PR description

we have ui state and widgets reading this ui state. This means that when you dispose a widget, you don't need to call that function if you start with a clean helper state instead of persisting the state.

This needs to be done in a major version, as people might rely on state without ui state (i'm not sure if they do) and that would be removed if they dispose a widget.

This allows again for classic react-instantsearch-core style virtual widgets for the modal and dynamic widget use case

This could be introduced non-breaking as a different "mode" InstantSearch behaves in, that could be swapped the default value later

codesandbox-ci · 2022-09-29T14:37:21Z

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 87d1851:

Sandbox	Source
InstantSearch.js	Configuration

src/widgets/index/index.ts

Haroenv · 2022-09-29T14:40:34Z

src/widgets/index/index.ts

@@ -450,7 +450,10 @@ const index = (widgetParams: IndexWidgetParams): IndexWidget => {
      };

      derivedHelper = mainHelper.derive(() =>
-        mergeSearchParameters(...resolveSearchParameters(this))
+        mergeSearchParameters(
+          mainHelper.state,


this part here is to ensure that "statically set parameters" on mainHelper persist (parameters on helper don't, as that actually is the helper of the mainIndex, confusing naming, I know).

Later, we probably should simply get rid of helper, and replace it with mainHelper only (which then gets name swapped)

codesandbox-ci · 2023-09-12T12:51:38Z

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 8ea7e44:

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

examples/react/getting-started/src/App.tsx

we have ui state and widgets reading this ui state. This means that when you dispose a widget, you don't need to call that function if you start with a clean helper state instead of persisting the state. This needs to be done in a major version, as people _might_ rely on state without ui state (i'm not sure if they do) and that would be removed if they dispose a widget. This allows again for classic react-instantsearch-core style virtual widgets for the modal and dynamic widget use case

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

Co-authored-by: Haroen Viaene <hello@haroen.me>

Haroenv · 2023-09-28T09:34:07Z

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

+    safelyRunOnBrowser(() => {
+      if (options.future?.preserveSharedStateOnUnmount === undefined) {


why not warn or warning? they already take a boolean that can be filled with options.future?.preserveSharedStateOnUnmount !== undefined

A warning might signify to users that not adopting the change will break their implementation, so I preferred a lighter informational message.

I'm mainly thinking of the DEV wrapping for warn. Not sure if info or warn makes much difference though, it's a valid point

Oh right, I didn't think about this and agree it should be scoped to dev mode. I'll add this.

…nt refinements snapshot

tests/common/widgets/current-refinements/options.ts

Haroenv

Not sure what the right solution is for warn/info but otherwise this lgtm

.codesandbox/ci.json

Haroenv · 2023-10-02T14:04:32Z

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

+    safelyRunOnBrowser(() => {
+      if (options.future?.preserveSharedStateOnUnmount === undefined) {


I'm mainly thinking of the DEV wrapping for warn. Not sure if info or warn makes much difference though, it's a valid point

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

Haroenv

✔️

sarahdayan · 2023-10-04T15:04:11Z

packages/instantsearch.js/src/widgets/index/__tests__/index-test.ts

Why not use this opportunity to create an tests/common/widgets/index/options.ts file and test this new behavior in it? No need to migrate the rest of our tests, we can do it separately, but at least we could start "the right way" and test as a user, without manipulating the helper.

Also, we should add a test for the default case (false, but not passed by the user).

There was a CTS-style test before, but we couldn't get it to pass, not sure if that's stashed anywhere

Yes, I've pushed the CTS here if you want to take a look, basically we saw lifecycle issues with React and Vue in the testing environment for which we could not find solutions for now.

Thanks for the suggestion on the missing test case, I've added it in 58ddb2a.

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

Haroenv commented Sep 29, 2022

View reviewed changes

src/widgets/index/index.ts Outdated Show resolved Hide resolved

Haroenv commented Sep 29, 2022

View reviewed changes

Haroenv added the requires major a change which could not be implemented without a backwards incompatibility label Sep 29, 2022

Haroenv force-pushed the wip/dispose-is-gone branch from a2e4077 to 1bb6531 Compare November 25, 2022 09:44

Haroenv commented Sep 13, 2023

View reviewed changes

examples/react/getting-started/src/App.tsx Outdated Show resolved Hide resolved

Haroenv mentioned this pull request Sep 25, 2023

fix: typo fixed in next app router docs #5860

Merged

Haroenv and others added 6 commits September 27, 2023 15:46

make it a flag

b2e99f2

rename flags to mode to prevent issue during build

13aaa5f

show dispose mode in a demo

424838f

dispose widgets in both modes

cca149b

refactor implementation and watch prop change on react/vue

9ff57f1

dhayab force-pushed the wip/dispose-is-gone branch from de926c3 to 9ff57f1 Compare September 27, 2023 13:56

revert changes on react getting-started example

d1184e7

dhayab changed the title ~~wip: no longer process dispose search parameters~~ feat: preverse shared state on unmount with a future flag Sep 27, 2023

dhayab changed the title ~~feat: preverse shared state on unmount with a future flag~~ feat: preserve shared state on unmount with a future flag Sep 27, 2023

dhayab removed the requires major a change which could not be implemented without a backwards incompatibility label Sep 27, 2023

dhayab requested review from dhayab and removed request for dhayab September 27, 2023 14:02

Haroenv commented Sep 27, 2023

View reviewed changes

packages/instantsearch.js/src/lib/InstantSearch.ts Outdated Show resolved Hide resolved

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

273e15c

Haroenv commented Sep 27, 2023

View reviewed changes

packages/instantsearch.js/src/lib/InstantSearch.ts Show resolved Hide resolved

dhayab and others added 2 commits September 28, 2023 11:27

add dev message

f0b5fec

Co-authored-by: Haroen Viaene <hello@haroen.me>

update bundlesize

f2f2248

Haroenv commented Sep 28, 2023

View reviewed changes

tests: report switch to mainHelper in insights tests and update curre…

9e23186

…nt refinements snapshot

Haroenv commented Sep 28, 2023

View reviewed changes

tests/common/widgets/current-refinements/options.ts Outdated Show resolved Hide resolved

Haroenv mentioned this pull request Sep 29, 2023

feat(useInstantSearch): expose renderState #5873

Merged

dhayab added 2 commits October 2, 2023 15:05

test new removeWidgets behaviour

9b6b3b9

revert changes not related to the PR

1164ee8

dhayab mentioned this pull request Oct 2, 2023

fix: use mainHelper consistently #5875

Merged

Merge branch 'master' into wip/dispose-is-gone

be7cccf

dhayab marked this pull request as ready for review October 2, 2023 13:59

dhayab requested a review from sarahdayan October 2, 2023 14:00

Haroenv commented Oct 2, 2023

View reviewed changes

dhayab added 3 commits October 2, 2023 16:27

do not show info message in prod

25ee2b6

revert change in .codesandbox/ci.json

7216293

finetune bundlesize

f260104

Haroenv commented Oct 2, 2023

View reviewed changes

packages/instantsearch.js/src/lib/InstantSearch.ts Outdated Show resolved Hide resolved

dhayab added 2 commits October 3, 2023 11:57

remove safelyRunOnBrowser

4dfa861

Merge branch 'master' into wip/dispose-is-gone

d67c60b

dhayab mentioned this pull request Oct 3, 2023

chore: release #5877

Closed

Haroenv commented Oct 3, 2023

View reviewed changes

sarahdayan reviewed Oct 4, 2023

View reviewed changes

add test when flag is unset

58ddb2a

dhayab requested a review from sarahdayan October 4, 2023 16:18

Merge branch 'master' into wip/dispose-is-gone

8ea7e44

sarahdayan approved these changes Oct 5, 2023

View reviewed changes

dhayab merged commit 2258d89 into master Oct 9, 2023
3 checks passed

dhayab deleted the wip/dispose-is-gone branch October 9, 2023 08:47

dhayab added a commit that referenced this pull request Oct 12, 2023

feat: preserve shared state on unmount with a future flag (#5123)

0d21f0d

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: preserve shared state on unmount with a future flag #5123

feat: preserve shared state on unmount with a future flag #5123

Haroenv commented Sep 29, 2022 •

edited by dhayab

Loading

codesandbox-ci bot commented Sep 29, 2022

Haroenv Sep 29, 2022 •

edited

Loading

codesandbox-ci bot commented Sep 12, 2023 •

edited

Loading

Haroenv Sep 28, 2023

dhayab Oct 2, 2023

Haroenv Oct 2, 2023

dhayab Oct 2, 2023

Haroenv left a comment

Haroenv Oct 2, 2023

Haroenv left a comment

sarahdayan Oct 4, 2023

Haroenv Oct 4, 2023

dhayab Oct 4, 2023

		safelyRunOnBrowser(() => {
		if (options.future?.preserveSharedStateOnUnmount === undefined) {

feat: preserve shared state on unmount with a future flag #5123

feat: preserve shared state on unmount with a future flag #5123

Conversation

Haroenv commented Sep 29, 2022 • edited by dhayab Loading

codesandbox-ci bot commented Sep 29, 2022

Haroenv Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

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

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

Haroenv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Haroenv 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

Haroenv commented Sep 29, 2022 •

edited by dhayab

Loading

Haroenv Sep 29, 2022 •

edited

Loading

codesandbox-ci bot commented Sep 12, 2023 •

edited

Loading