Repository navigation
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🦋 Changeset detectedLatest commit: 4d666dd The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff27ec7464
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.signal.throwIfAborted(); | ||
| if (!this.initialized) { | ||
| this.initialized = true; | ||
| for (const source of this.sources) { |
There was a problem hiding this comment.
Given that MergedAsyncIterator is only constructed in a Symbol.asyncIterator call, creating the inner iterators synchronously seems better.
Can we move the initializer to run in the constructor? In the unlikely event of [Symbol.asyncIterator]() itself throwing, I think we should report that synchronously instead of leaving MergedAsyncIterator in a weird half state.
|
|
||
| function wrapPromiseWithIndex<T>(promise: Promise<T>, index: number) { | ||
| return promise.then((value) => ({ value, index })) as Promise<MergeResult<T>>; | ||
| export function mergeAsyncIterables<T>(sources: IterableSource<T>[], signal?: AbortSignal): AsyncIterable<T> { |
There was a problem hiding this comment.
Nit: Outside of tests, we always pass an abort signal to this. So I wonder if it would be cleaner to mark it as required, but that doesn't seem to simplify much. So probably fine as-is.
This is a rewrite of
MergedAsyncIteratorto avoid issues with returning.This is currently used for two use cases:
Conceptually, when we merge two iterators, if one closes we need to close the other one. But with how iterators work in JavaScript, if we call
return()on an iterator while it is already blocking on anext(), that return waits for the next to complete first. So the current implementation did not block onreturn(), since it could result in deadlocks. The caller is then responsible for ensuring both iterators are aborted afterwards. An example of such an issue is in #773. The the non-blocking return meant that an uncaught error inreturn()could result in an uncaught rejection, that kills the process.This re-designs the API slightly: The
MergedAsyncIteratornow passes in a signal to each iterator. When theMergedAsyncIteratorcloses, it aborts the sources directly, meaning it can safely awaitreturn. It does mean that each source must act on the AbortSignal to abort when requested.There are no actual known issues with uncaught rejections fixed by this - it is merely a better structuring of the APIs.
This now also removes the
ixdependency, which was barely used. The remaining use cases were mostly itsAbortError, which we now replace with aDOMExceptionto be consistent with the errors used byAbortController. This also improves propagation of the underlyingreasonon abort errors, but that does not have an actual improvement in logs that I could find.AI Usage
Implemented using Codex gpt-6.1-sol; manually verified the approach and implementation.