Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Remove AggregateError.prototype.toString #49

Merged
merged 1 commit into from
Nov 11, 2019
Merged

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Nov 11, 2019

Refs #36

Copy link
Member

@mathiasbynens mathiasbynens left a comment

Choose a reason for hiding this comment

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

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.

@devsnek devsnek marked this pull request as ready for review November 11, 2019 02:33
Copy link
Collaborator

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

@bakkot Great catch!

@chicoxyzzy chicoxyzzy merged commit 0fe86b6 into tc39:master Nov 11, 2019
@chicoxyzzy
Copy link
Member

Thank you!

@devsnek devsnek deleted the patch-1 branch November 11, 2019 06:34
zloirock added a commit to zloirock/core-js that referenced this pull request Nov 11, 2019
@anba
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
Copy link
Member

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
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants