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

Add pMapIterable #63

Merged
merged 25 commits into from
Dec 5, 2023
Merged

Add pMapIterable #63

merged 25 commits into from
Dec 5, 2023

Conversation

Richienb
Copy link
Contributor

@Richienb Richienb commented Dec 30, 2022

Main differences from pMap():

  • Skipped values are simply never yielded
  • stopOnError is not available because this can now be done by the user themselves
  • signal is also not available for the same reason
  • Values that are yielded from the iterator, are immediately passed to the mapper and the returned values are yielded from the function in the order that they originally were yielded

If you're ok with this, I'll add docs and tests

Fixes sindresorhus/promise-fun#21

index.js Outdated
(async () => {
for (let index = 0; index < concurrency; index++) {
// eslint-disable-next-line no-await-in-loop
await nextItem();
Copy link
Owner

Choose a reason for hiding this comment

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

What if nextItem throws? It will cause an unhandled rejection.

index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Bump :)

Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
@sindresorhus
Copy link
Owner

What's left to do here?

@Richienb
Copy link
Contributor Author

What's left to do here?

docs

Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
@Richienb Richienb marked this pull request as ready for review July 14, 2023 04:34
@sindresorhus
Copy link
Owner

Why do we need the separate backpressure option?

@Richienb
Copy link
Contributor Author

Richienb commented Jul 16, 2023

Why do we need the separate backpressure option?

Values returned by the async iterator can only be consumed one at a time. However, we can run the mapper function concurrency times in parallel. This means there will end up being a backlog of values that are waiting to be consumed. We limit the size of the backlog using backpressure such that we don't allow further calls to the mapper function if doing so would cause it to exceed the backpressure.

@Richienb
Copy link
Contributor Author

Might be a good idea to try re-implement pMap using pMapIterable to simplify the code and see where its holes are.

index.d.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Values returned by the async iterator can only be consumed one at a time. However, we can run the mapper function concurrency times in parallel. This means there will end up being a backlog of values that are waiting to be consumed. We limit the size of the backlog using backpressure such that we don't allow further calls to the mapper function if doing so would cause it to exceed the backpressure.

👍

Richienb and others added 4 commits July 23, 2023 04:25
Co-authored-by: Sindre Sorhus <[email protected]>
Co-authored-by: Sindre Sorhus <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
@Richienb
Copy link
Contributor Author

What should happen when the mapper function returns an error?
pMap simply throws the error because it only returns the values when everything finishes. However, pMapIterable streams returned values. Should it just prevent future values down the iterable from running and let the ones before it finish?

@sindresorhus
Copy link
Owner

An error should generally make things stop as soon as possible, so I don't think it should even let the current ones finish.

@Richienb
Copy link
Contributor Author

Richienb commented Aug 2, 2023

Ok I guess I'll throw out values that arrived out of order after an error.

@Richienb
Copy link
Contributor Author

@sindresorhus I did stuff

@Richienb Richienb marked this pull request as draft November 23, 2023 15:09
@Richienb Richienb marked this pull request as ready for review November 23, 2023 15:10
index.js Outdated Show resolved Hide resolved
test.js Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Nice! Big improvement.

test.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@Richienb Richienb marked this pull request as draft November 26, 2023 03:13
@Richienb Richienb marked this pull request as ready for review November 26, 2023 03:13
index.js Outdated Show resolved Hide resolved
@sindresorhus sindresorhus merged commit 5c59528 into sindresorhus:main Dec 5, 2023
2 checks passed
@sindresorhus
Copy link
Owner

Nice work 🙏

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.

p-prefetch or p-iterable?
2 participants