Implement Promise Race Without Promise Group #889
Conversation
komyg
force-pushed
the
feature/promise-race-without-promise-group
branch
from
285befa to
145149d
Compare
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
Mostly looks good to me, but we can simplify the Race case a little, and I think we should separate the Race entirely out of Group.
...m/src/ecmascript/builtins/control_abstraction_objects/promise_objects/promise_constructor.rs
Outdated
Show resolved
Hide resolved
...m/src/ecmascript/builtins/control_abstraction_objects/promise_objects/promise_constructor.rs
Outdated
Show resolved
Hide resolved
...m/src/ecmascript/builtins/control_abstraction_objects/promise_objects/promise_constructor.rs
Outdated
Show resolved
Hide resolved
...m/src/ecmascript/builtins/control_abstraction_objects/promise_objects/promise_constructor.rs
Outdated
Show resolved
Hide resolved
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
Nice, looks excellent! IMO you could still remove the extra HeapMarkAndSweep impl when you update expectations, but aside from that this is now complete as far as I can see.
Great job and thank you <3
komyg
force-pushed
the
feature/promise-race-without-promise-group
branch
from
15fc83f to
ba69f41
Compare
Thanks! :) Unfortunately, it seems that I broke quite a few tests from the other promise constructor methods, such as I have to check why before merging... |
komyg
force-pushed
the
feature/promise-race-without-promise-group
branch
from
49ea3e3 to
c46c861
Compare
|
Hey @aapoalas, I fixed the abrupt rejection that I broke, so this should be good to go. Could you review again when you have the time, please? |
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
Hey @aapoalas, I fixed the abrupt rejection that I broke, so this should be good to go.
Good that you got it fixed, nice work!
I had one nitpick regarding the way you handle the rejections, let's fix that and then we can merge this <3
...m/src/ecmascript/builtins/control_abstraction_objects/promise_objects/promise_constructor.rs
Outdated
Show resolved
Hide resolved
Fixed! :) |
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
LGTM, great job and thank you <3
2 participants
No description provided.