fix(onStateChange): propagate change to middleware

[Haroenv]

This wrong behaviour was originally introduced in #4699 by removing onInternalStateChange, since I assumed the helper state change event would call the middleware, but it doesn't happen in all cases.

This means that now onInternalStateChange gets called one more time in certain situations, but that's not really an issue, as it's deferred anyway.

I've checked and the integration test written by @tkrugg indeed starts failing on #4699

Fixes #4795

[codesandbox-ci]

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 89ca17f:

Sandbox	Source
InstantSearch.js	Configuration

[tkrugg]

It looks good, thanks for fixing this so fast.
The regression was that onInternalStateChange was no longer called in setUiState. We should have a test checking this specifically (middleware.onStateChange not being called was just a side effect of the regression).

[Haroenv]

that's more of a side-effect of what actually was the wrong behaviour (middleware weren't called), the onInternalStateChange could change without breaking the behaviour @tkrugg, we shouldn't have a test on the solution, but on the problem it solves

[tkrugg]

that's more of a side-effect of what actually was the wrong behaviour (middleware weren't called), the onInternalStateChange could change without breaking the behaviour @tkrugg, we shouldn't have a test on the solution, but on the problem it solves

I think we agree on the idea but disagree on what we call side-effect and behaviour.
I'm really speaking from the setUiState point of view (because that's where I think we should add a test).
The behaviour of this public function is "change uiState and notify everybody with uiState has changed".
The regression happened because the second part of this responsibility was removed, and there was nothing that could have prevented it. Such a test would have made it very clear.

middleware being notified is the side-effect. If at some point we decide middleware will be notified by another mean, setUiState test doesn't have to know about it, it's not its responsibility. I don't think we can ever go wrong by having a test clearly checking the behaviour of a public function is preserved, and here its behaviour is not "it should call middleware", it's "it should notify others of UI State update". That's why I think we're missing a test.

[Haroenv]

There's actually already two tests here, but they passed with the bug already, do you have any idea what's missing in the condition @tkrugg? https://github.com/algolia/instantsearch.js/blob/e8329aecb386755a039cf10850e394d0d71f29f4/src/lib/__tests__/InstantSearch-test.tsx#L1769-L1856

[eunjae-lee]

The new 2 test cases make sense to me. Which scenario do you think we should test more? @tkrugg

[tkrugg]

I was thinking one that would check onInternalStateUpdate is called when setUIState is, because that's the "notify everybody state has changed" part of the implem.

[eunjae-lee]

I was thinking one that would check onInternalStateUpdate is called when setUIState is, because that's the "notify everybody state has changed" part of the implem.

yeah, but also the implementation can change while the end result stays the same.

The contract we have is

when setUiState is called(A), the middlewares' onStateChange gets called(B).

It's not important what happens between (A) and (B). We trigger (A) and see if (B) is achieved. By testing this way, we have a room to refactor , even rename or do whatever with the internal implementations.