-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
Allow stopping iteration via pMap.stop
#36
Changes from all commits
778a7da
cd0343a
e70cd78
941ed8b
3dcb6d0
90a42b0
31be4be
be46075
02cc500
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
declare const stop: unique symbol; | ||
|
||
export type StopSymbol = typeof stop; | ||
|
||
export interface Options { | ||
/** | ||
Number of concurrently pending promises returned by `mapper`. | ||
|
@@ -16,6 +20,46 @@ export interface Options { | |
readonly stopOnError?: boolean; | ||
} | ||
|
||
export interface OngoingMappingsStopOptions { | ||
/** | ||
Whether or not to remove all holes from the result array (caused by pending mappings). | ||
|
||
@default false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any thoughts on making this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fine by me. I don't have any strong opinion. Personally, I will probably set this option explicitly whenever I use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Richienb do you have any opinion on what would be the best default value here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets change it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agree, to match its behaviour with |
||
*/ | ||
readonly collapse?: boolean; | ||
|
||
/** | ||
Value to use as immediate result for pending mappings, replacing holes from the result array. | ||
|
||
This option is ignored if `collapse` is set to `true`. | ||
|
||
@default undefined | ||
*/ | ||
readonly fillWith?: unknown; | ||
} | ||
|
||
export interface StopOptions<NewElement> { | ||
/** | ||
Value to provide as result for this iteration. | ||
|
||
@default undefined | ||
*/ | ||
readonly value?: NewElement; | ||
|
||
/** | ||
Options to configure what `pMap` must do with any concurrent ongoing mappings at the moment `stop` is called. | ||
*/ | ||
readonly ongoingMappings?: OngoingMappingsStopOptions; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if there's any way we could flatten the options object:
Needs better names. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it makes sense to group them. In the future, if support for canceling promises and Do you also want better names for the way I've currently done it? Or you said "better names" only for your suggestion itself? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe something like this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think the non-flattened way is better, because
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think this particular behavior could be left for another PR. I've never seen good uses for holes in arrays, they're an obscure feature of the language, if I'm not mistaken TypeScript for example pretends they don't exist. Sindre also said it here. Although if you found a good use case for them, I'd be interested in knowing more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, we can stick with the nested option. I'm not really sold on most of these names though. The problem is that it applies to any mappings, not just running ones, but also pending ones. For example, if the |
||
} | ||
|
||
type BaseStopValueWrapper<NewElement> = { | ||
[stop]: Required<StopOptions<NewElement>>; | ||
}; | ||
|
||
export type StopValueWrapper<NewElement> = NewElement extends any ? BaseStopValueWrapper<NewElement> : never; | ||
|
||
type MaybeWrappedInStop<NewElement> = NewElement | StopValueWrapper<NewElement>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why these 3 types? Not immediately clear why this complication is needed. We don't need to expose anything about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
To be honest that was the simplest way I could make it work. All attempts to simplify this further caused a test in
Nice idea. Do you have any favorite way of declaring an opaque object in TypeScript? Recenly I've seen There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Actually, on second thought, I think we have to, because as soon as the user uses Let's merge it this way, currently, and you can open an issue to improve typings? Then if someone finds a way to do it without breaking the current TS tests I added, it can be done later. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a stop value is recieved, it could set a variable which causes future resolutions of other items to avoid adding new values or creating new calls. Then, the array can be processed and resolved. |
||
|
||
/** | ||
Function which is called for every item in `input`. Expected to return a `Promise` or value. | ||
|
||
|
@@ -25,7 +69,7 @@ Function which is called for every item in `input`. Expected to return a `Promis | |
export type Mapper<Element = any, NewElement = unknown> = ( | ||
element: Element, | ||
index: number | ||
) => NewElement | Promise<NewElement>; | ||
) => MaybeWrappedInStop<NewElement> | Promise<MaybeWrappedInStop<NewElement>>; | ||
|
||
/** | ||
@param input - Iterated over concurrently in the `mapper` function. | ||
|
@@ -54,8 +98,39 @@ console.log(result); | |
//=> ['https://sindresorhus.com/', 'https://avajs.dev/', 'https://github.com/'] | ||
``` | ||
*/ | ||
export default function pMap<Element, NewElement>( | ||
input: Iterable<Element>, | ||
mapper: Mapper<Element, NewElement>, | ||
options?: Options | ||
): Promise<NewElement[]>; | ||
declare const pMap: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of defining a constant do this: export function stop(...);
export default function pMap(...); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests failed because the way the function is imported changed: - import pMap from 'p-map';
- console.log(pMap, pMap.stop);
+ import pMap, {stop} from 'p-map';
+ console.log(pMap, stop); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per c9c0882, the symbol should be a named export called |
||
<Element, NewElement>( | ||
input: Iterable<Element>, | ||
mapper: Mapper<Element, NewElement>, | ||
options?: Options | ||
): Promise<NewElement[]>; | ||
|
||
/** | ||
Creates a special value that should be returned from `mapper` in order to immediately stop iteration. | ||
|
||
@example | ||
``` | ||
import pMap from 'p-map'; | ||
import got from 'got'; | ||
|
||
const numbers = Array.from({length: 2000}).map((_, index) => index + 1); | ||
//=> [1, 2, ..., 1999, 2000] | ||
|
||
const mapper = async number => { | ||
if (number !== 404) { | ||
const {transcript} = await got(`https://xkcd.com/${number}/info.0.json`).json(); | ||
if (/unicorn/.test(transcript)) { | ||
console.log('Found a XKCD comic with an unicorn:', number); | ||
return pMap.stop(); | ||
} | ||
} | ||
}; | ||
|
||
await pMap(numbers, mapper, {concurrency: 50}); | ||
//=> Found a XKCD comic with an unicorn: 948 | ||
``` | ||
*/ | ||
stop: <NewElement>(options?: StopOptions<NewElement>) => StopValueWrapper<NewElement>; | ||
}; | ||
|
||
export default pMap; |
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.
I don't see
StopSymbol
used anywhere.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.
Based on your idea above of not exposing the existence of
{ [stop] }
in the returned object, perhaps this is indeed not needed. The reason I first added this was to have a way to expose it to the user, since for example in find-up it is also exposed.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.
Should I undo this, then?
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.
Yes
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.
What do you think about passing options param for exposing stop in the returned object?