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

Normative: Date.prototype.toString throws on non-Date receiver #850

Merged
merged 1 commit into from
Jun 28, 2017

Conversation

littledan
Copy link
Member

JSC and V8 have been shipping the version of the specification
included in this patch, so it is likely to be web-compatible.
Throwing on receivers which are not Date instances is simpler
and more consistent with other methods in ECMAScript.

Closes #849

JSC and V8 have been shipping the version of the specification
included in this patch, so it is likely to be web-compatible.
Throwing on receivers which are not Date instances is simpler
and more consistent with other methods in ECMAScript.

Closes tc39#849
@littledan littledan added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Mar 18, 2017
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Awesome, consider me a vote for

@leobalter
Copy link
Member

+1, we might need to update test262, thou, but that's easy and fast.

@leobalter
Copy link
Member

20.3.4.41: Previous editions did not specify the value returned by Date.prototype.toString when this time value is NaN. ECMAScript 2015 specifies the result to be the String value is "Invalid Date".

From section/annex D, should this be changed too?

@littledan littledan removed the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Mar 22, 2017
@littledan
Copy link
Member Author

Tests in tc39/test262#924

@anba
Copy link
Contributor

anba commented Apr 5, 2017

I think the current behaviour is based on the discussion in https://mail.mozilla.org/pipermail/es-discuss/2014-June/037630.html.

@littledan
Copy link
Member Author

@anba Huh, based on the negative replies there, and the fact that RegExp was not similarly updated (until I developed evidence of web compatibility issues and patched it for ES2016), I assumed that this thread wasn't the one that led to the change.

Anyway, although V8 received bug reports about RegExp.prototype.toString() throwing, I don't see any bug reports about Date.prototype.toString() throwing. I'm not sure how much of a user issue this is. Note that many other methods on Date throw on Date.prototype, and conversion to a string throws for many other objects in the system, e.g., Symbol.prototype.

@anba
Copy link
Contributor

anba commented Apr 5, 2017

Huh, based on the negative replies there, and the fact that RegExp was not similarly updated (until I developed evidence of web compatibility issues and patched it for ES2016), I assumed that this thread wasn't the one that led to the change.

I think this is the thread that led to the current Date.prototype.toString method, at least based on when it took place. The messages are from June 2014 and in the next ES6 draft (rev 26), @allenwb added

+ 1. Let Q be this Date object.
+ 2. If Type(O) is Object and O does not have a [[DateValue]] internal slot, then
+   a. Let tv be NaN.
+ 3. Else,
   a. Let tv be this time value.
 4. Return ToDateString(tv)

to Date.prototype.toString.

Anyway, although V8 received bug reports about RegExp.prototype.toString() throwing, I don't see any bug reports about Date.prototype.toString() throwing. I'm not sure how much of a user issue this is.

If the change is web-compatible, we probably should try to apply it.

@rwaldron rwaldron added has consensus This has committee consensus. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 and removed has consensus This has committee consensus. needs consensus This needs committee consensus before it can be eligible to be merged. labels May 25, 2017
@leobalter
Copy link
Member

tests in tc39/test262#924

@leobalter leobalter removed the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Jun 12, 2017
@leobalter
Copy link
Member

@bterlson :shipit:

@littledan
Copy link
Member Author

IIRC we got consensus on this change in the May 2017 TC39 meeting. There are tests. Is this ready to merge?

@bterlson
Copy link
Member

Sorry I didn't see @leobalter's earlier comment. With tests, this is ready to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants