Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds a new allKeyed helper to the promise utilities, mirroring the TC39 Promise.allKeyed proposal semantics by resolving an object’s promise values concurrently while preserving keys—plus exports, tests, and multi-language docs.
Changes:
- Implement
src/promise/allKeyed.tsto resolve keyed promise/value objects viaPromise.all. - Add
vitestcoverage for core behaviors (empty input, rejection, mixed values, concurrency). - Re-export from
src/promise/index.tsand add reference docs (en/ko/ja/zh_hans).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/promise/index.ts | Re-exports allKeyed from the promise module entrypoint. |
| src/promise/allKeyed.ts | Implements allKeyed concurrency + key-preserving result shaping. |
| src/promise/allKeyed.spec.ts | Adds unit tests for resolution/rejection/mixed inputs + concurrency behavior. |
| docs/reference/promise/allKeyed.md | English reference documentation for allKeyed. |
| docs/ko/reference/promise/allKeyed.md | Korean reference documentation for allKeyed. |
| docs/ja/reference/promise/allKeyed.md | Japanese reference documentation for allKeyed. |
| docs/zh_hans/reference/promise/allKeyed.md | Simplified Chinese reference documentation for allKeyed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const result = {} as { [K in keyof T]: Awaited<T[K]> }; | ||
|
|
||
| for (let i = 0; i < keys.length; i++) { | ||
| result[keys[i]] = values[i] as Awaited<T[(typeof keys)[number]]>; | ||
| } |
There was a problem hiding this comment.
Building result as a normal object and assigning result[key] = ... can trigger the special __proto__ setter when the input contains a __proto__ key, changing the returned object's prototype (prototype pollution risk / surprising behavior). Consider using the existing _internal/isUnsafeProperty guard and Object.defineProperty for __proto__ (similar to toPlainObject), or otherwise ensure __proto__ becomes a normal data property.
| it('should resolve promises concurrently, not sequentially', async () => { | ||
| const start = Date.now(); | ||
|
|
||
| await allKeyed({ | ||
| a: new Promise(resolve => setTimeout(() => resolve('a'), 50)), | ||
| b: new Promise(resolve => setTimeout(() => resolve('b'), 50)), | ||
| }); | ||
|
|
||
| const elapsed = Date.now() - start; | ||
|
|
||
| // If run sequentially, would take ~100ms. Concurrently should be ~50ms. | ||
| expect(elapsed).toBeLessThan(90); | ||
| }); |
There was a problem hiding this comment.
The concurrency assertion relies on real wall-clock timing (Date.now() and setTimeout) with a fairly tight threshold, which is likely to be flaky under CI load. Consider using Vitest fake timers (vi.useFakeTimers() / advanceTimersByTime) or at least a more robust timing approach (e.g., performance.now() + a looser bound) so the test validates concurrency without depending on scheduler jitter.
| it('should return an empty object for empty input', async () => { | ||
| const result = await allKeyed({}); | ||
|
|
||
| expect(result).toEqual({}); | ||
| }); | ||
|
|
||
| it('should reject if any promise rejects', async () => { | ||
| await expect( | ||
| allKeyed({ | ||
| a: Promise.resolve(1), | ||
| b: Promise.reject(new Error('fail')), | ||
| }) | ||
| ).rejects.toThrow('fail'); | ||
| }); |
There was a problem hiding this comment.
Given the library already has prototype-pollution hardening elsewhere (e.g., skipping/handling __proto__), it would be good to add a test case where the input contains a __proto__ key and assert it becomes an own enumerable property (and does not alter the returned object's prototype). This will prevent regressions once the implementation is hardened.
Summary
allKeyedfunction based on the TC39Promise.allKeyedproposal (Stage 2.7)Promise.all, but accepts an object instead of an array — making it easy to destructure results by nameChanges
src/promise/allKeyed.ts— Implementationsrc/promise/allKeyed.spec.ts— Tests (7 cases)src/promise/index.ts— Re-exportTest plan
yarn vitest run src/promise/allKeyed)Closes #1593