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

README Suggestion #191

Open
dgh500 opened this issue Jun 2, 2021 · 1 comment
Open

README Suggestion #191

dgh500 opened this issue Jun 2, 2021 · 1 comment

Comments

@dgh500
Copy link

dgh500 commented Jun 2, 2021

Just a suggestion - in the readme gotchas section it says:

limiter.schedule(() => {
  const allTasks = tasksArray.map(x => processTask(x));
  // GOOD, we wait until all tasks are done.
  return Promise.all(allTasks);
});

When I initially read this I read this as meaning you could wrap .schedule around a Promise.all and it would throttle the requests within the Promise.all, which upon further reading and testing isn't the case and I ended up with the following as part of a larger function.

await Promise.allSettled(
   skus.map(async (sku) => {
     return await limiter.schedule(async () => {
        return await updateMonthlySalesForSku(sku);
      });
   })
)

The use of allSettled isn't relevant in this instance however the use of "// GOOD, we wait until all tasks are done." in the gotcha documentation gave me the impression that the async calls should all be wrapped by the .schedule function.

The documentation overall is very clear, just this example in particular I feel could be a little clearer.

@am1ru1
Copy link

am1ru1 commented Nov 5, 2022

can you PR?
your explanation is more in-line with the expectation of promise.all

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

No branches or pull requests

2 participants