fix(core): run getSources promises in a concurrent-safe way

[francoischalifour]

Description

This fixes an issue where promises coming from getSources or getSuggestions resolve out of call order.

+----------------------------------+
|        100ms                     | 
| run(1) +--->  R1                 |
|        300ms                     |
| run(2) +-------------> R2 (SKIP) |
|        200ms                     |
| run(3) +--------> R3             |
+----------------------------------+

In the scenario above, although R2 is the result resolving last, we shouldn't update the results because R3 is more fresh (although it resolved earlier).

Related

• Internal feedback on the Documentation Search v2
• https://twitter.com/MisterAwesan/status/1318138123977674753

---

cc @redox @Shipow

[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 bf6a07f:

Sandbox	Source
@algolia/js-example	Configuration

[redox]

That's great! I like how clear you made (and tested) this. I'll let the team figure out whether it's mergeable.

[eunjae-lee]

If I understand correctly, don't we need to have a difference instance of createConcurrentSafePromise per source here? For each source, we want to keep the last result, not the last result of all the sources.
Let me know if I'm misunderstanding!

[francoischalifour]

Yeah exactly. This case is complex because of the dynamic nature of sources. This PR doesn't cover this case anymore.

[Haroenv]

I need to look even more in detail to the core usage of this, but the util lgtm

[francoischalifour]

The last commits make sure that the concurrency fix works at the getSources level.

It's however not fixed for getItems because we can't keep a same reference to create the wrapper on. This is a limitation that I propose we tackle in another PR.

[eunjae-lee]

Looks good to me. The concurrency test is super clear.

[sarahdayan]

Maybe add a test to check for the presence of { label: 'a' } as well as the first item?

[Haroenv]

maybe a snapshot of all the labels could make sense?

[francoischalifour]

A snapshot will be fragile because there's no certainty of how many times onStateChange is called. It's called whenever a setter is called (e.g. setIsOpen, etc.), and a setter can be called user-land. It means that if we refactor something, this test will become a false negative.

I'll check for the presence of a though.