Skip to content
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

Implement ES2021 Promise.any #1610

Merged
merged 2 commits into from
Sep 12, 2024
Merged

Implement ES2021 Promise.any #1610

merged 2 commits into from
Sep 12, 2024

Conversation

camnwalter
Copy link
Contributor

@camnwalter camnwalter commented Sep 4, 2024

  • AggregateError/errors-iterabletolist-failures fails because get [Symbol.iterator]() doesn't work (not too sure why)
    • throws TypeError: The object is not a string
  • AggregateError/message-tostring-abrupt and AggregateError/message-tostring-abrupt-symbol both fail
    • This is due to how ScriptRuntime.toString(), which calls ScriptableObject.getDefaultValue() doesn't take into account the [Symbol.toPrimitive] property, it only checks for toString() or valueOf(). See ScriptableObject.getDefaultValue and the spec
  • AggregateError/newtarget-proto-fallback fails because we don't support new.target
  • Promise/any/reject-element-function-property-order fails. Other tests checking the order of length and name seem to fail as well

Fixes #1062

@camnwalter
Copy link
Contributor Author

One thing I realized is that AggregateError should throw a type error if not supplied with anything as it's first parameter or if is isn't iterable... Can't think of anything else yet

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 5, 2024

Great stuff once again! And tnx for digging into the reasons behind the failing tests as well!

Wrt the get [Symbol.iterator]() issue: looks like an issue with the computed properties implementation:

js> var o = {get [Symbol.iterator]() {}}
js: "<stdin>", line 2: uncaught JavaScript runtime exception: TypeError: The object is not a string
	at <stdin>:2

js> var s = Symbol.iterator
js> var o = {get [s]() {}}
js: "<stdin>", line 4: uncaught JavaScript runtime exception: TypeError: The object is not a string
	at <stdin>:4

js> var p = 'something'
js> var o = {get [p]() {}} // No error here
js> var o = {[Symbol.iterator]() {}} // No error here

Could you create follow-up cases for:

  • supporting symbols in Computed Properties icw get
  • taking into account Symbol.toPrimitive in places where it's needed?
  • the issue you identified wrt to length & name property ordering?

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 5, 2024

One thing I realized is that AggregateError should throw a type error if not supplied with anything as it's first parameter or if is isn't iterable...

Not sure I get what you mean here. Are you saying that AggregateErrors ought to behave like this according to the spec, but your implementation currently doesn't and that is not caught by the test262 test suite?

@camnwalter
Copy link
Contributor Author

On Firefox, if you do AggregateError() or AggregateError(something not iterable), it throws that at least 1 argument is required and the first parameter isn't iterable, respectively. I'm not sure if this is according to spec, since it looks like the errors field should still be populated, but will just be empty.

https://tc39.es/ecma262/#sec-aggregate-error

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 5, 2024

Following the referenced operations in the spec, I come to the conclusion that a TypeError should be thrown if the argument doesn't resolve to an iterator, so what FF does seems correct to me

@camnwalter
Copy link
Contributor Author

Ah I see, will add that. Should I add test cases for that? Haven't done that before.

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 5, 2024

If test262 doesn't cover this scenario, you might file a case with them for this. If they act upon it quickly, we can just update to the newer version of test262 to get test coverage. Otherwise, a test ought to be added yes.

I'm in favor of going the test262 route

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 5, 2024

The second issue, about toPrimitive will likely be fixed by PR #1611 that was just created

@camnwalter
Copy link
Contributor Author

Could you create follow-up cases for:

* taking into account Symbol.toPrimitive in places where it's needed?
const object1 = {
  [Symbol.toPrimitive]() {
    return "toPrimitive"
  },
  toString() {
    return "toString"
  },
  valueOf() {
    return "valueOf"
  }
};

> String(object1);

In Rhino:

valueOf

In FF:

toPrimitive

https://tc39.es/ecma262/#sec-tostring

* the issue you identified wrt to length & name property ordering?
> function foo(x, y, z) {}
> Object.getOwnPropertyNames(foo)

In Rhino:

arguments,prototype,name,arity,length

In FF:

prototype,length,name

The ordering is defined in https://tc39.es/ecma262/#sec-createbuiltinfunction

@gbrail
Copy link
Collaborator

gbrail commented Sep 11, 2024

This looks good to me-- any other comments, otherwise we should merge this.

@p-bakker
Copy link
Collaborator

Note that the latest test262 now has a test for the AggregateError constructor throwing an error if the first param is not supplied

@gbrail
Copy link
Collaborator

gbrail commented Sep 12, 2024

Thanks -- I think that this is far enough that we can merge it, and we can follow up later to update test262.

@gbrail gbrail merged commit 44532cc into mozilla:master Sep 12, 2024
3 checks passed
@camnwalter camnwalter deleted the promise-any branch September 12, 2024 14:51
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.

Support ES2021 Promise.any
3 participants