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

fix(concurrency): ensure panel stays closed after blur #829

Merged
merged 30 commits into from Dec 9, 2021

Conversation

sarahdayan
Copy link
Member

@sarahdayan sarahdayan commented Nov 24, 2021

This fixes a concurrency issue with closing the panel by running an additional tracked promise without triggering a search.

Problem

When a user types a query that triggers network requests (for example, to Algolia), these requests could sometimes take longer to resolve (e.g., the service is slow, the network is slow). In the meantime, the user could decide to manually close the panel. However, a late resolving request could reopen the panel.

This doesn't make sense from a functional perspective: if you've purposefully closed the panel, it should be considered a cancellation.

The issue comes from late promises in onInput performing asynchronous state updates that cancel previous ones. This is a similar problem to #654.

Solution

Since we can't avoid asynchronous and potentially late state updates, we can schedule another one when we close the panel by calling onInput with { nextState: isOpen: false }. When a user closes the panel, it creates a new concurrent-safe promise that will follow any running ones and close the panel.

To avoid triggering an extra request when we do this, we make sure to return early (before calling getSources) in those situations.

This PR introduces a new isRunning private method on the function returned from createConcurrentSafePromise to know whether there are running promises. This is useful to control when we need to skip a search, or when to schedule the extra onInput call.

submit.mov

I've left a couple inline review comments to explain some implementation details.

Next steps

No major new concept is introduced to fix this issue, but using onInput to schedule a later state update is a workaround. In the future, if we need to do this elsewhere, we might want to introduce a generic helper to do it (e.g., onAsyncUpdate(maybePromise, nextState)).

fix #806

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 24, 2021

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

Sandbox Source
@algolia/autocomplete-example-github-repositories-custom-plugin Configuration
@algolia/autocomplete-example-instantsearch Configuration
@algolia/autocomplete-example-playground Configuration
@algolia/autocomplete-example-preview-panel-in-modal Configuration
algolia/autocomplete Configuration
@algolia/autocomplete-example-starter-algolia Configuration
@algolia/autocomplete-example-starter Configuration
@algolia/autocomplete-example-reshape Configuration
algolia/autocomplete Configuration
@algolia/autocomplete-example-react-renderer (forked) Issue #806

Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

The solution at the promise level is fine to me, but the onInput calls increase too much the complexity, and will make it hard to not introduce bugs in the future. We'd be very prone to forgetting when to call it, and what next state to pass to it.

I propose a change on top of that branch in #831. It removes a lot of complexity because we don't need to think in terms of "what should be the next state after cancelling this interaction", but rather just "let's cancel this interaction". The bundle size is also liter.

Most of the comments that I added in this PR thread become irrelevant with the proposed solution.

@sarahdayan If that solution seems better to your too, feel free to merge this on your branch, or to cherry pick the commit.

Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

Do you want to apply the patches from #831?

packages/autocomplete-core/src/onInput.ts Outdated Show resolved Hide resolved
packages/autocomplete-core/src/getPropGetters.ts Outdated Show resolved Hide resolved
sarahdayan and others added 2 commits December 6, 2021 18:41
Co-authored-by: François Chalifour <francoischalifour@users.noreply.github.com>
Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

A few last comments and we're good.

packages/autocomplete-core/src/getPropGetters.ts Outdated Show resolved Hide resolved
packages/autocomplete-core/src/createStore.ts Outdated Show resolved Hide resolved
packages/autocomplete-core/src/getPropGetters.ts Outdated Show resolved Hide resolved
packages/autocomplete-core/src/onKeyDown.ts Show resolved Hide resolved
packages/autocomplete-core/src/getPropGetters.ts Outdated Show resolved Hide resolved
Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

Awesome – that's solid now!

@sarahdayan sarahdayan merged commit 2dd34e0 into next Dec 9, 2021
@sarahdayan sarahdayan deleted the fix/late-promises-on-submit branch December 9, 2021 13:25
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.

Panel reopens after submitting the query with late resolving promises
2 participants