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

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

Merged
merged 3 commits into from
Jan 24, 2023
Merged

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Jan 18, 2023

Summary

going a step further than #5429, we want to also when there's an error revert to the last known good state.

Result

This goes back to the previous (non-optimistic) behaviour when an error happens, with a caveat that the state will be fully consistent. That means UiState and the URL also update back to the previous state

@Haroenv Haroenv changed the base branch from master to feat/optimistic-rendering January 18, 2023 17:33
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 18, 2023

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 72f2b16:

Sandbox Source
InstantSearch.js Configuration
react-instantsearch-app Configuration

Comment on lines +589 to +598
// we can't attach a listener to the error event of search, as the error
// then would no longer be thrown for global handlers.
if (
instantSearchInstance.status === 'error' &&
!instantSearchInstance.mainHelper!.hasPendingRequests()
) {
helper!.setState(lastValidSearchParameters!);
}

Copy link
Contributor Author

@Haroenv Haroenv Jan 20, 2023

Choose a reason for hiding this comment

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

I wonder if this can be put somewhere else? I thought in InstantSearch error handler, but that doesn't know local state etc, so would require a dedicated method on index widget.

I guess this could be problematic if render is called another time after error, with error again, it will again reset. (which is a little superfluous, but still correct)

@Haroenv Haroenv changed the title WIP: realistic rendering feat(rendering): revert search state on error Jan 20, 2023
@Haroenv Haroenv force-pushed the feat/revert-on-error branch from 2e30ce2 to dd65aff Compare January 23, 2023 11:14
@Haroenv Haroenv marked this pull request as ready for review January 23, 2023 11:14
@Haroenv Haroenv requested review from a team, FabienMotte and sarahdayan and removed request for a team January 23, 2023 11:15
@Haroenv Haroenv force-pushed the feat/revert-on-error branch from 8ea34e7 to c5c21e5 Compare January 23, 2023 15:43
Base automatically changed from feat/optimistic-rendering to master January 23, 2023 16:11
Haroenv added a commit that referenced this pull request Jan 23, 2023
**Summary**

This change enables optimistic rendering, as discussed in [RFC 82](algolia/instantsearch-rfcs#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 

Co-authored-by: Dhaya <154633+dhayab@users.noreply.github.com>
Co-authored-by: Sarah Dayan <sarahdayan@users.noreply.github.com>
@Haroenv Haroenv force-pushed the feat/revert-on-error branch from 5662e8c to ac9d640 Compare January 23, 2023 16:25
@Haroenv Haroenv force-pushed the feat/revert-on-error branch from cc02675 to 72f2b16 Compare January 23, 2023 16:28
@Haroenv Haroenv mentioned this pull request Jan 24, 2023
@dhayab dhayab self-requested a review January 24, 2023 15:47
Copy link
Member

@dhayab dhayab left a comment

Choose a reason for hiding this comment

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

Good for me!

@Haroenv Haroenv merged commit 732fcac into master Jan 24, 2023
@Haroenv Haroenv deleted the feat/revert-on-error branch January 24, 2023 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants