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

πŸ“ Rework section on waitAll in the tutorial #4482

Merged
merged 2 commits into from Nov 27, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -35,16 +35,12 @@ pendingQueries.push(queued(2).then((v) => seenAnswers.push(v)));
await s.waitFor(Promise.all(pendingQueries));
```

An alternative options would have been to use `waitAll` instead of `waitFor` but it comes with a precise requirements being that promise had to be imperatively scehduled by the time we reach it. In other words, if our code delays a little bit the call to `call` it will not wait anything. With the current implementation of `queue` it works, but with next versions of it it might not be the case anymore as queued calls might start queuing themselves behind already started and running promises.
An alternative would have been to use `waitAll` instead of `waitFor` but it comes with a precise requirement: promises have to be already scheduled by the time we request the scheduler. In other words, if our code delays a little bit the call to `call`, the scheduler might not wait enough.

As the fact that `call` has to be fired synchronously when issuing a call on a clean context is not a requirement for our current function, we can relax the constraint in our test to make evolving this implementation easier.
Given the fact that `call` should be fired synchronously is not a requirement for our current function, we can relax the constraint in our test to make evolving this implementation easier.
dubzzz marked this conversation as resolved.
Show resolved Hide resolved

A `waitAll` version of the code above would be:

Alternatively, we could use `waitAll` instead of `waitFor`, but this approach requires promises to be scheduled by the time we reach `waitAll`. If our code introduces any delays, `waitAll` may not wait for anything. While the current implementation of `queue` works with `waitAll`, future versions may not, as queued calls might start queuing behind already running promises meaning that promises might be scheduled after calling `waitAll`. Therefore, we can relax the requirement that `call` get fired synchronously in our test, to make the implementation easier to evolve.

While not as good as the `waitFor` implementation, here's the `waitAll` version of the code:

```js
const queued = queue(s.scheduleFunction(call));
queued(1).then((v) => seenAnswers.push(v));
Expand Down