-
Notifications
You must be signed in to change notification settings - Fork 332
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
Conversation
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:
|
… are still unresolved
There was a problem hiding this 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.
packages/autocomplete-core/src/utils/createConcurrentSafePromise.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-core/src/utils/__tests__/createConcurrentSafePromise.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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?
Co-authored-by: François Chalifour <[email protected]>
There was a problem hiding this 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.
There was a problem hiding this 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!
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 fromcreateConcurrentSafePromise
to know whether there are running promises. This is useful to control when we need to skip a search, or when to schedule the extraonInput
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