Allow stopping iteration via `pMap.stop` by papb · Pull Request #36 · sindresorhus/p-map

papb · 2021-06-05T23:59:17Z

Closes #31

~~Note: this PR also currently includes a bug fix for an edge case, see below.~~ edit: no longer true, moved to #40

sindresorhus · 2021-06-07T15:39:27Z

+
+export type StopValueWrapper<NewElement> = NewElement extends any ? BaseStopValueWrapper<NewElement> : never;
+
+type MaybeWrappedInStop<NewElement> = NewElement | StopValueWrapper<NewElement>;


Why these 3 types? Not immediately clear why this complication is needed. We don't need to expose anything about stop to the user in the return value. For the user, it can just be an opaque object.

Why these 3 types? Not immediately clear why this complication is needed.

To be honest that was the simplest way I could make it work. All attempts to simplify this further caused a test in test-d.ts to fail. In particular, I observed that TypeScript does not realize automatically that { x: A } | { x: B } is the same as { x: A | B }, so I had to write this extends any ? hack.

For the user, it can just be an opaque object.

Nice idea. Do you have any favorite way of declaring an opaque object in TypeScript? Recenly I've seen Record<never, never>, does this sound good?

We don't need to expose anything about stop to the user in the return value. For the user, it can just be an opaque object.

Actually, on second thought, I think we have to, because as soon as the user uses return pMap.stop() inside mapper, TypeScript will infer the return type of mapper differently, and we need TS to infer it to something that we can unwrap later in order to provide a correct return type for the pMap call itself. This was quite complicated to achieve to be honest. I don't see how it would be possible if we simplify the result of .stop to a constant type such as Record<never, never>.

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?

Can't you just do what was done here: c9c0882 (Exclude the symbol from the return type)

// @Richienb

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.

sindresorhus · 2021-06-07T15:39:55Z

@@ -1,3 +1,7 @@
+declare const stop: unique symbol;
+
+export type StopSymbol = typeof stop;


I don't see StopSymbol used anywhere.

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.

Should I undo this, then?

Should I undo this, then?

Yes

What do you think about passing options param for exposing stop in the returned object?

sindresorhus · 2021-06-07T15:47:10Z

+	/**
+	Whether or not to remove all holes from the result array (caused by pending mappings).
+
+	@default false


Any thoughts on making this true by default?

That's fine by me. I don't have any strong opinion. Personally, I will probably set this option explicitly whenever I use .stop, regardless of what is the default. Do you want me to change it to true?

@Richienb do you have any opinion on what would be the best default value here?

Lets change it to true then.

Lets change it to true then.

Agree, to match its behaviour with pMapSkip which could have left holes but didn't.

sindresorhus · 2021-06-07T15:52:03Z

+	/**
+	Options to configure what `pMap` must do with any concurrent ongoing mappings at the moment `stop` is called.
+	*/
+	readonly ongoingMappings?: OngoingMappingsStopOptions;


I wonder if there's any way we could flatten the options object:

value

collpasePending

fillPending

Needs better names.

I think it makes sense to group them. In the future, if support for canceling promises and AbortController is added, they might probably need options that could also be added to this group.

Do you also want better names for the way I've currently done it? Or you said "better names" only for your suggestion itself?

Maybe something like this:

value Value to resolve with

defaultValue Value to set when the promise hasn't been run
What if the consumer wants it to be an empty value? Maybe this could be in the form of passing a pMap.stop.empty symbol

stopRunning Whether currently running promises should be allowed to finish

removeIncomplete Whether empty items in the resulting array where there should have been values if all the promises had run should be removed.

I still think the non-flattened way is better, because value is about the current iteration, while everything else is about configuring what happens to the rest of the mapping itself. Maybe instead of ongoingMappings, one of:

rest

restConfig

restOptions

whatToDoWithOngoingMappings

mappingsStillRunning

inProgressBehavior

inProgressConfig

What if the consumer wants it to be an empty value? Maybe this could be in the form of passing a pMap.stop.empty symbol

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.

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 concurrency option is used, not all remaining mappings are executing when it's stopped. I think the only one that is usable is rest, so I guess we can go with that.

sindresorhus · 2021-06-07T15:53:18Z

@Richienb Would you be able to help review? 🙏

Richienb · 2021-06-09T08:55:58Z

-	mapper: Mapper<Element, NewElement>,
-	options?: Options
-): Promise<NewElement[]>;
+declare const pMap: {


Instead of defining a constant do this:

export function stop(...); export default function pMap(...);

Hello, I tried to do it this way on 941ed8b but TypeScript tests failed, so I reverted it via 31be4be, what do you think?

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);

Per c9c0882, the symbol should be a named export called pMapStop.

Richienb · 2021-06-09T08:58:24Z


+### pMap.stop(options?)
+
+Creates a special object that indicates to `pMap` that iteration must stop immediately. This object should just be returned from within the mapper (and not used directly for anything).


Suggested change

Creates a special object that indicates to `pMap` that iteration must stop immediately. This object should just be returned from within the mapper (and not used directly for anything).

Returns a value that can be returned from the `mapper` function provided to `pMap` to immediately stop iteration.

I further reworded it taking inspiration in your text: be46075 - what do you think?

Richienb · 2021-06-09T09:15:44Z

+	/**
+	Options to configure what `pMap` must do with any concurrent ongoing mappings at the moment `stop` is called.
+	*/
+	readonly ongoingMappings?: OngoingMappingsStopOptions;


Maybe something like this:

value Value to resolve with

defaultValue Value to set when the promise hasn't been run
What if the consumer wants it to be an empty value? Maybe this could be in the form of passing a pMap.stop.empty symbol

stopRunning Whether currently running promises should be allowed to finish

removeIncomplete Whether empty items in the resulting array where there should have been values if all the promises had run should be removed.

This reverts commit 941ed8b.

sindresorhus · 2021-06-30T09:23:47Z

@@ -1,3 +1,7 @@
+declare const stop: unique symbol;
+
+export type StopSymbol = typeof stop;


Should I undo this, then?

Yes

sindresorhus · 2021-06-30T09:24:36Z

+	/**
+	Whether or not to remove all holes from the result array (caused by pending mappings).
+
+	@default false


Lets change it to true then.

sindresorhus · 2021-06-30T09:26:41Z

-	mapper: Mapper<Element, NewElement>,
-	options?: Options
-): Promise<NewElement[]>;
+declare const pMap: {


Per c9c0882, the symbol should be a named export called pMapStop.

sindresorhus · 2021-06-30T09:30:38Z

+
+export type StopValueWrapper<NewElement> = NewElement extends any ? BaseStopValueWrapper<NewElement> : never;
+
+type MaybeWrappedInStop<NewElement> = NewElement | StopValueWrapper<NewElement>;


Can't you just do what was done here: c9c0882 (Exclude the symbol from the return type)

// @Richienb

sindresorhus · 2021-06-30T09:38:06Z

+	/**
+	Options to configure what `pMap` must do with any concurrent ongoing mappings at the moment `stop` is called.
+	*/
+	readonly ongoingMappings?: OngoingMappingsStopOptions;


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 concurrency option is used, not all remaining mappings are executing when it's stopped. I think the only one that is usable is rest, so I guess we can go with that.

sindresorhus · 2021-09-07T10:34:17Z

@papb Friendly bump in case you forgot about this PR. Feel free to ignore if you're just busy ;)

sindresorhus · 2022-12-10T20:14:52Z

Closing to let someone else be able to work on this as this PR is not moving forward.

papb force-pushed the early-stop branch from 8c2411c to 38471ba Compare June 6, 2021 00:00

Allow stopping iteration via pMap.stop

778a7da

papb force-pushed the early-stop branch from 38471ba to 778a7da Compare June 6, 2021 00:01

papb commented Jun 6, 2021

View reviewed changes

Comment thread index.js

sindresorhus requested changes Jun 7, 2021

View reviewed changes

Richienb suggested changes Jun 9, 2021

View reviewed changes

papb added 4 commits June 26, 2021 15:20

refactor finished to isFinished

cd0343a

Manual linting of examples

e70cd78

Refactor index.d.ts away from export const

941ed8b

Improve wording in readme

3dcb6d0

papb mentioned this pull request Jun 26, 2021

Do not run mapping after stop-on-error happened #40

Merged

papb added 4 commits June 26, 2021 15:40

Fix typo in variable name

90a42b0

Revert "Refactor index.d.ts away from export const"

31be4be

This reverts commit 941ed8b.

Improve description of the stop function

be46075

Undo change moved to another PR

02cc500

sindresorhus requested changes Jun 30, 2021

View reviewed changes

sindresorhus mentioned this pull request Dec 10, 2022

Allow breaking from iterator #31

Open

sindresorhus closed this Dec 10, 2022


		export type StopValueWrapper<NewElement> = NewElement extends any ? BaseStopValueWrapper<NewElement> : never;

		type MaybeWrappedInStop<NewElement> = NewElement \| StopValueWrapper<NewElement>;

		@@ -1,3 +1,7 @@
		declare const stop: unique symbol;

		export type StopSymbol = typeof stop;


		### pMap.stop(options?)

		Creates a special object that indicates to `pMap` that iteration must stop immediately. This object should just be returned from within the mapper (and not used directly for anything).

	Creates a special object that indicates to `pMap` that iteration must stop immediately. This object should just be returned from within the mapper (and not used directly for anything).
	Returns a value that can be returned from the `mapper` function provided to `pMap` to immediately stop iteration.

Uh oh!

Conversation

papb commented Jun 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

papb Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Richienb Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

papb Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus commented Jun 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

papb commented Jun 5, 2021 •

edited

Loading

papb Jun 7, 2021 •

edited

Loading

Richienb Jun 30, 2021 •

edited

Loading

papb Jun 7, 2021 •

edited

Loading