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

Remove AggregateError.prototype.toString #49

Merged
merged 1 commit into from Nov 11, 2019
Merged

Conversation

@devsnek
Copy link
Member

devsnek commented Nov 11, 2019

Refs #36

Copy link
Member

mathiasbynens left a comment

LGTM. (This section was originally added because the first draft meant to specify the error message differently. However, instead this should be done through the already-implementation-defined value of the message property, which is a change we made later. Nowadays, the section indeed seems redundant.)

I’ll wait for @chicoxyzzy’s review before merging.

@mathiasbynens mathiasbynens requested review from chicoxyzzy and bakkot Nov 11, 2019
@devsnek devsnek marked this pull request as ready for review Nov 11, 2019
@bakkot
bakkot approved these changes Nov 11, 2019
Copy link
Collaborator

bakkot left a comment

Note that the two functions are not totally identical, in that

let error = new AggregateError([]);
error.name = void 0;
error.toString();

would prior to this change have given "AggregateError" and after it will give "Error".

I don't think anyone will ever notice that without starting from the spec, though, and since no other error type has that behavior anyway this seems an improvement.

@mathiasbynens

This comment has been minimized.

Copy link
Member

mathiasbynens commented Nov 11, 2019

@bakkot Great catch!

@ljharb
ljharb approved these changes Nov 11, 2019
@chicoxyzzy chicoxyzzy merged commit 0fe86b6 into tc39:master Nov 11, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@chicoxyzzy

This comment has been minimized.

Copy link
Member

chicoxyzzy commented Nov 11, 2019

Thank you!

@devsnek devsnek deleted the devsnek:patch-1 branch Nov 11, 2019
zloirock added a commit to zloirock/core-js that referenced this pull request Nov 11, 2019
@anba

This comment has been minimized.

Copy link

anba commented Nov 11, 2019

CC @leobalter because test262 requires updates after this change.

The following tests probably need to be updated:

  • test262/built-ins/AggregateError/prototype/toString/get-message-undefined.js
  • test262/built-ins/AggregateError/prototype/toString/get-name-empty-string.js
  • test262/built-ins/AggregateError/prototype/toString/get-name-undefined.js
  • test262/built-ins/AggregateError/prototype/toString/prop-desc.js

And while there these two tests also need fixes (they call AggregateError without an iterable argument, resulting in throwing a TypeError).

  • test262/built-ins/AggregateError/newtarget-proto-custom.js
  • test262/built-ins/AggregateError/newtarget-proto-fallback.js

These six tests failed when running test262 with the Promise.any patch for SpiderMonkey (currently under review).

@leobalter

This comment has been minimized.

Copy link
Member

leobalter commented Nov 11, 2019

I'm removing tests for AggregateError#toString and adding one that asserts instances will point to Error#toString.

leobalter added a commit to leobalter/test262 that referenced this pull request Nov 11, 2019
leobalter added a commit to leobalter/test262 that referenced this pull request Nov 11, 2019
leobalter added a commit to leobalter/test262 that referenced this pull request Nov 11, 2019
@leobalter

This comment has been minimized.

Copy link
Member

leobalter commented Nov 11, 2019

@anba New patches for Test262 should fix these. PTAL?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.