fix: make promise throttle level consistent #240
Conversation
@W-13709025@
| @@ -18,6 +18,15 @@ export type PromiseItem<T, O = T | undefined> = { | |||
| producer: (source: T, throttledPromise: ThrottledPromiseAll<T, O | undefined>) => Promise<O | undefined>; | |||
| }; | |||
|
|
|||
| type IndexedProducer<T, O = T> = PromiseItem<T, O> & { | |||
There was a problem hiding this comment.
Internal use type to add index to input and output.
Index is used to maintain result order to align results with Promise.all
| @@ -56,7 +65,7 @@ export class ThrottledPromiseAll<T, O = T> { | |||
| * Returns the results of the promises that have been resolved. | |||
| */ | |||
| public get results(): Array<O | undefined> { | |||
| return this.#results; | |||
| return this.#results.sort((a, b) => (a?.index ?? 0) - (b?.index ?? 0)).map((r) => r?.result); | |||
There was a problem hiding this comment.
Return the results in same order supplied by user
| // eslint-disable-next-line no-await-in-loop | ||
| const results = await Promise.all( | ||
| next.map((item) => item.producer(item.source, this).catch((e) => Promise.reject(e))) | ||
| const generator = function* ( |
There was a problem hiding this comment.
Makes code a bit more concise
src/throttledPromiseAll.ts
Outdated
| const concurrencyPool: Array<Promise<IndexedResult<O> | undefined>> = []; | ||
| const get = generator(this.queue); | ||
| let index = 0; | ||
| while (this.queue.length > 0 || concurrencyPool.length > 0) { |
There was a problem hiding this comment.
Stay here while there are things to do.
src/throttledPromiseAll.ts
Outdated
| const get = generator(this.queue); | ||
| let index = 0; | ||
| while (this.queue.length > 0 || concurrencyPool.length > 0) { | ||
| while (concurrencyPool.length < this.concurrency) { |
There was a problem hiding this comment.
Make sure there are always running promises at the concurrency level
| break; | ||
| } | ||
|
|
||
| const p: IndexedProducer<T, O> = { ...item, index: index++ }; |
There was a problem hiding this comment.
Add in the index property to the supplied producer
| concurrencyPool.push( | ||
| p | ||
| .producer(item.source, this) | ||
| .then((result) => ({ index: p.index, result })) |
There was a problem hiding this comment.
capture the index in the result
src/throttledPromiseAll.ts
Outdated
| } | ||
| this.#results.push( | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await Promise.race([concurrencyPool.shift(), ...concurrencyPool]) |
There was a problem hiding this comment.
Race until one finishes.
peternhale
changed the title
| yield data.shift(); | ||
| } | ||
| }; | ||
| const concurrencyPool: Map<number, Promise<IndexedResult<O> | undefined>> = new Map< |
There was a problem hiding this comment.
Make pool a map, which simplifies the removal of pool entries that have resolved.
|
QA notes: checked using plugin-dev's |
src/throttledPromiseAll.ts
Outdated
| @@ -56,7 +65,8 @@ export class ThrottledPromiseAll<T, O = T> { | |||
| * Returns the results of the promises that have been resolved. | |||
| */ | |||
| public get results(): Array<O | undefined> { | |||
| return this.#results; | |||
| const results = this.#results; | |||
There was a problem hiding this comment.
what's the point of this line? new const is just a reference to the class prop.
Maybe you want [...results].sort() if you're trying to avoid accidental mutations?
There was a problem hiding this comment.
I'll fix it. Debug junk left over
@W-13709025@