Polyfill: clarify error message about why valueOf throws
#2667
Conversation
This file contains 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
justingrant
added
non-prod-polyfill
Codecov Report
@@ Coverage Diff @@
## main #2667 +/- ##
=======================================
Coverage 96.07% 96.08%
=======================================
Files 20 20
Lines 11706 11723 +17
Branches 2186 2188 +2
=======================================
+ Hits 11247 11264 +17
Misses 395 395
Partials 64 64
|
gibson042
reviewed
Aug 26, 2023
| @@ -5726,6 +5726,21 @@ export function ASCIILowercase(str) { | |||
| ]); | |||
| } | |||
|
|
|||
| export function ValueOfThrows(constructorName) { | |||
There was a problem hiding this comment.
Isn't ecmascript.mjs intended to correspond with operations that are in current ECMA-262? It seems like the wrong home for this function.
There was a problem hiding this comment.
ecmascript.mjs is usually where we put all shared code. Unlike the spec files where there's mainadditions.html for the existing 262 overrides, in the polyfill things are all mixed together. That said, this function isn't in the spec so I'll add a comment explaining why it exists.
gibson042
reviewed
Aug 26, 2023
Comment on lines
+5735
to
+5743
| throw new TypeError( | ||
| 'Do not use built-in arithmetic operators with Temporal objects. ' + | ||
| `When comparing, use ${compareCode}, not obj1 > obj2. ` + | ||
| "When coercing to strings, use obj.toString() or String(obj), not +obj nor '' + obj. " + | ||
| 'When concatenating with strings, use str.concat(obj) or `${str}${obj}`, not str + obj. ' + | ||
| 'In React, call obj.toString() before rendering a Temporal object.' | ||
| ); |
There was a problem hiding this comment.
That's a lot of quote variance. For sanity, I think maybe just ` with post-processing would be better.
Suggested change
| throw new TypeError( | |
| 'Do not use built-in arithmetic operators with Temporal objects. ' + | |
| `When comparing, use ${compareCode}, not obj1 > obj2. ` + | |
| "When coercing to strings, use obj.toString() or String(obj), not +obj nor '' + obj. " + | |
| 'When concatenating with strings, use str.concat(obj) or `${str}${obj}`, not str + obj. ' + | |
| 'In React, call obj.toString() before rendering a Temporal object.' | |
| ); | |
| const rawMessage = ` | |
| Do not use built-in arithmetic operators with Temporal objects. | |
| When comparing, use ${compareCode} rather than obj1 > obj2. | |
| When coercing to string, use obj.toString() or String(obj) rather than '' + obj. | |
| When concatenating to a string, use str.concat(obj) or \`\${str}\${obj}\` rather than str + obj. | |
| In React, call obj.toString() before rendering a Temporal object. | |
| `; | |
| const trimmed = Call(StringPrototypeTrim, rawMessage, []); | |
| const singleLine = Call(StringPrototypeReplace, trimmed, [/\s+/g, ' ']); | |
| throw new TypeError(singleLine); |
There was a problem hiding this comment.
The quote variance comes from prettier; it's automatic. And anyone who's seen a lot of prettier code shouldn't be surprised by this. Also, I'm not convinced that this change would add much extra readability. So I'm inclined to merge as-is. Holler if you strongly disagree!
We've received enough user confusion about valueOf throwing that it's worth clarifying the error message.
justingrant
force-pushed
the
fix-react-error
branch
from
b8af801 to
946a9a4
Compare
justingrant
added a commit
that referenced
this pull request
Sep 29, 2023
Continue where #2667 left off by further clarifying this error message and fixing a mistake where `+foo` was treated as a string coercion, not the number coercion that it is.
justingrant
added a commit
that referenced
this pull request
Sep 29, 2023
Continue where #2667 left off by further clarifying this error message and fixing a mistake where `+foo` was treated as a string coercion, not the number coercion that it is.
Ms2ger
pushed a commit
that referenced
this pull request
Oct 2, 2023
Continue where #2667 left off by further clarifying this error message and fixing a mistake where `+foo` was treated as a string coercion, not the number coercion that it is.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We've received enough user confusion about
valueOfthrowing (see #1681 for why we throw) that I think it's worth clarifying the error message.