Skip to content

Conversation

@komyg
Copy link
Contributor

@komyg komyg commented Oct 4, 2025

No description provided.

@komyg komyg force-pushed the feature/promise.all_settled branch from 6271720 to 16237ab Compare October 11, 2025 11:10
@komyg komyg requested a review from aapoalas October 14, 2025 12:38
@komyg komyg marked this pull request as ready for review October 14, 2025 12:38
@komyg komyg force-pushed the feature/promise.all_settled branch from a68dc67 to fae782c Compare October 14, 2025 12:41
Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this looks great! Only one real blocking nit to pick, and then a couple of suggestions on reducing some repetition in the resolving code.

Absolutely stellar stuff! <3


let data = promise_all.get_mut(agent);

// i. Set remainingElementsCount.[[Value]] to remainingElementsCount.[[Value]] - 1.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I think we can probably factor out this part and make it a method of the PromiseGroupRecord: take &mut self, reduce remaining_elements_count by 1, and return (Array, Option<Promise>) with the Promise only getting returned if remaining_elements_count == 0. Now the caller can do an assignment into the Array with their value and then resolve the Promise if they received one.

Also note: this way the code doesn't need to first read the result array from the PromiseGroupRecord, then write into that array, and then go back to reading from the PromiseGroupRecord to check the remaining elements count and promise. It can do all of those reads at the same time.

@komyg komyg force-pushed the feature/promise.all_settled branch from bbf3be7 to 295b9eb Compare October 27, 2025 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants