-
Notifications
You must be signed in to change notification settings - Fork 903
Implement ES2024 Array.fromAsync #2076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
anivar
wants to merge
5
commits into
mozilla:master
Choose a base branch
from
anivar:es2024-array-from-async
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+972
−2
Open
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
16305ee
Implement ES2024 Array.fromAsync
anivar a90e701
Implement ES2024 Array.fromAsync
anivar 890fa94
Implement ES2024 Array.fromAsync with sequential promise processing
anivar 1214356
Simplify Array.fromAsync to handle only array-like objects
anivar b1e770d
Merge upstream master and update test262 properties for Array.fromAsync
anivar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
83 changes: 83 additions & 0 deletions
83
tests/src/test/java/org/mozilla/javascript/tests/es2024/ArrayFromAsyncTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| package org.mozilla.javascript.tests.es2024; | ||
|
|
||
| import org.junit.Test; | ||
| import org.mozilla.javascript.testutils.Utils; | ||
|
|
||
| /** | ||
| * Tests for ES2024 Array.fromAsync | ||
| * | ||
| * <p>Note: Full async testing requires manual promise queue processing in Rhino. These tests verify | ||
| * basic functionality. | ||
| */ | ||
| public class ArrayFromAsyncTest { | ||
|
|
||
| @Test | ||
| public void testArrayFromAsyncExists() { | ||
| String script = "typeof Array.fromAsync === 'function';"; | ||
| Utils.assertWithAllModes_ES6(true, script); | ||
| } | ||
|
|
||
| @Test | ||
| public void testArrayFromAsyncReturnsPromise() { | ||
| String script = "Array.fromAsync([1, 2, 3]) instanceof Promise;"; | ||
| Utils.assertWithAllModes_ES6(true, script); | ||
| } | ||
|
|
||
| @Test | ||
| public void testArrayFromAsyncWithEmptyArray() { | ||
| String script = "Array.fromAsync([]) instanceof Promise;"; | ||
| Utils.assertWithAllModes_ES6(true, script); | ||
| } | ||
|
|
||
| @Test | ||
| public void testArrayFromAsyncWithPromiseArray() { | ||
| String script = | ||
| "Array.fromAsync([Promise.resolve(1), Promise.resolve(2)]) instanceof Promise;"; | ||
| Utils.assertWithAllModes_ES6(true, script); | ||
| } | ||
|
|
||
| @Test | ||
| public void testArrayFromAsyncWithIterable() { | ||
| String script = | ||
| "var iterable = {" | ||
| + " [Symbol.iterator]: function* () { yield 1; yield 2; }" | ||
| + "};" | ||
| + "Array.fromAsync(iterable) instanceof Promise;"; | ||
| Utils.assertWithAllModes_ES6(true, script); | ||
| } | ||
|
|
||
| @Test | ||
| public void testArrayFromAsyncWithArrayLike() { | ||
| String script = | ||
| "var arrayLike = { 0: 'a', 1: 'b', length: 2 };" | ||
| + "Array.fromAsync(arrayLike) instanceof Promise;"; | ||
| Utils.assertWithAllModes_ES6(true, script); | ||
| } | ||
|
|
||
| @Test | ||
| public void testArrayFromAsyncWithMapper() { | ||
| String script = | ||
| "Array.fromAsync([1, 2], function(x) { return x * 2; }) instanceof Promise;"; | ||
| Utils.assertWithAllModes_ES6(true, script); | ||
| } | ||
|
|
||
| @Test | ||
| public void testArrayFromAsyncWithMapperAndThis() { | ||
| String script = | ||
| "var obj = { multiplier: 2 };" | ||
| + "Array.fromAsync([1, 2], function(x) { return x * this.multiplier; }, obj) instanceof Promise;"; | ||
| Utils.assertWithAllModes_ES6(true, script); | ||
| } | ||
|
|
||
| @Test | ||
| public void testArrayFromAsyncLength() { | ||
| String script = "Array.fromAsync.length === 1;"; | ||
| Utils.assertWithAllModes_ES6(true, script); | ||
| } | ||
|
|
||
| @Test | ||
| public void testArrayFromAsyncName() { | ||
| String script = "Array.fromAsync.name === 'fromAsync';"; | ||
| Utils.assertWithAllModes_ES6(true, script); | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The promise constructor only requires that its argument is a
Callable, so you could just pass method reference rather than use a whole separate subclass ofBaseFunctionhere, but I wonder if it would be better hold off a little on this and work out how we want to represent async built in functions, there are bound to be far more added to the spec over the next few years.I’m also not convinced this algorithm matches the draft spec, it’s not clear to me that it has the right behaviour in error cases, but I’ll need to find time to sit down with this, the test cases, and the draft spec to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aardvark179 You're absolutely right on all three points.
Agreed - using BaseFunction subclasses is unnecessary overhead. LambdaFunction or direct Callable is the correct pattern, as established in NativePromise (Promise.try, Promise.finally).
The Promise.all approach is fundamentally wrong. I used it for simplicity given Rhino's lack of async/await, but it violates the ES2024 spec which requires sequential processing like for await...of. This affects execution order, error handling, and resource consumption.
Current implementation doesn't match spec - it processes all values before handling errors, when it should stop at the first error (lazy) evaluation).
Why This Approach Was Taken
Without async/await or for-await-of in Rhino, I opted for Promise.all as the path of least resistance. This was a mistake - correctness should trump implementation simplicity.
Other approach is replacing Promise.all with recursive promise chaining:, Replace both AsyncFromExecutor and AsyncFromHandler with LambdaFunction.
Given the complexity of implementing ES2024 async semantics without async/await, Which of these following approach would you prefer ?
A: Fix now with correct but complex manual promise chainingOption
B: Create the reusable async pattern first, then apply it hereOption
C:Defer until Rhino has broader async support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this proposal is still in draft, so I see no strong need to rush to get an implementation in, and therefore I don’t think we should do A. I think B or C are probably the options to go for, with B seeming quite approachable, but using it as a lever to get us closer to C.
So, what would an async pattern for built-ins look like? I think, given this function is async because it must itself wait for results, then a generator returning promises that use
thento trigger the next step would work well, and so I think the very first step is to be able to expresss a generator.I think the model we currently for compiled generators (numbered re-entry points and a mutable object used to capture variable state) could be adapted nicely for asynchronous built-in functions, but if we are going to do some refactoring of that code I’d love to see us unify and reduce our generator implementations rather than add new ones.
If you want to try this then try refactoring the interpreter and compiler generator implementations to share the same model, and we can see how close that gets us. It will be hard on master at the moment, but #2091 goes some way towards this and might be easier to build upon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aardvark179 You are right. waiting for PR #2091 to land would provide a much better foundation