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

Add emu-note about why valueOf throws #1681

Closed
justingrant opened this issue Aug 2, 2021 · 12 comments
Closed

Add emu-note about why valueOf throws #1681

justingrant opened this issue Aug 2, 2021 · 12 comments
Assignees
Labels
editorial spec-text Specification text involved
Milestone

Comments

@justingrant
Copy link
Collaborator

js-temporal/temporal-polyfill#21 (comment) exposed an issue we should look at. The problem @12wrigja found is that when string interpolation is polyfilled by TS (and perhaps by Babel too?) it's converted to +, which per https://tc39.es/proposal-temporal/#sec-temporal.plaindate.prototype.valueof will throw.

throw new RangeError(Offset ${offsetStr} is invalid for ${dt} in ${timeZoneString}); is transpiled by tsc to throw new RangeError('Offset ' + offsetStr + ' is invalid for ' + dt + ' in ' + timeZoneString);, and the '+' operator invokes valueOf.

This implies that code using Temporal instances in string interpolations will work fine in native ES6 but will throw when polyfilled into ES5. Is this OK? If not, are there any ways around it (e.g. by using @@toPrimitive) ?

@ljharb
Copy link
Member

ljharb commented Aug 2, 2021

Babel handles this properly with the strict transpilation, but not with the loose one.

This is a transpiler bug when present; it’s fixable by them and the proposal shouldn’t be concerned with it (beyond filing issues etc)

@justingrant
Copy link
Collaborator Author

@ljharb what is the "correct" transpilation behavior in this case?

@ljharb
Copy link
Member

ljharb commented Aug 2, 2021

Wrapping it in String(), effectively - iirc Babel actually uses the String concat method to ensure proper spec semantics.

@gilmoreorless
Copy link
Contributor

There's an open issue for TypeScript about this transpilation behaviour at microsoft/TypeScript#39744. I've added a comment about the Temporal use case there.

@12wrigja
Copy link
Contributor

12wrigja commented Aug 4, 2021

Speaking of the spec, would it be possible to elaborate on the spec why these objects throw in their valueOf methods? https://tc39.es/proposal-temporal/#sec-temporal.plaindate.prototype.valueof isn't particularly informative.

@ptomato
Copy link
Collaborator

ptomato commented Aug 5, 2021

Meeting, August 5: the reasoning for valueOf throwing might be a good thing to add in an <emu-note>.

@justingrant
Copy link
Collaborator Author

FYI, I opened a PR against React to fix the problem there.

I'm starting to worry that this behavior will cause a lot of problems in the wild, given how often we've seen it so far with mainstream platforms. If we decided to not throw in valueOf, what bad things would happen?

@ptomato
Copy link
Collaborator

ptomato commented Aug 10, 2021

The discussion is in #517. (tl;dr: if you include a valueOf, then people will use < and > whether we like it or not. If valueOf returns the same thing as toString, then this will "work" in some cases, not in other cases, but enough of the time to be dangerous.)

The new angle here is actually not that people are using comparison operators, but that they are (indirectly, via a library or a transpiler) using the + operator for string concatenation which first consults valueOf instead of toString. My opinion is that libraries that do this are going to be broken for other cases, not just Temporal objects, so it's actually good if this surfaces and causes bugs in libraries to be fixed, which would otherwise remain latent.

In the thread in #517 there was another suggestion: having valueOf return a primitive type (likely a BigInt) such that the comparison operators always worked. The disadvantage of this was that cross-type comparisons, e.g. instant > plainDate would succeed but produce a nonsense result. For the string concatenation problem, I think it would actually make the problem worse, because you'd get wrong strings instead of exceptions, so it would still be broken but less obviously.

(For example, with the TypeScript bug, you'd get something like this:)

date = new Temporal.PlainDate(1970, 1, 1);
console.log(`The date is ${date}`);
// JavaScript: The date is 1970-01-01
// TypeScript: The date is 0

@justingrant
Copy link
Collaborator Author

Another detail to add: one of the reasons for < and + to throw is that it'd leave space for a better implementation if JS ever supported overloaded operators in the (distant?) future.

@ljharb
Copy link
Member

ljharb commented Aug 25, 2021

I would not hold my breath for that to work out smoothly.

@12wrigja
Copy link
Contributor

I think this can be marked as fixed given that both Babel and TypeScript now handle this correctly - TS is fixed as of v4.5.

@ptomato
Copy link
Collaborator

ptomato commented Dec 13, 2021

I think there was still a pending action to add an <emu-note> to each valueOf() algorithm explaining why it needs to throw.

@ptomato ptomato added spec-text Specification text involved editorial labels May 10, 2022
@ptomato ptomato changed the title Polyfilled string interpolation will call valueOf, which will throw Add emu-note about why valueOf throws May 10, 2022
@ptomato ptomato added this to the Stage 4 milestone Dec 8, 2022
@ptomato ptomato self-assigned this Mar 26, 2024
ptomato added a commit that referenced this issue Mar 26, 2024
These notes clarify in the spec text why valueOf() must throw, which is a
request we've had. It also suggests what implementations might put in the
error message to point users in the right direction.

Closes: #1681
@Ms2ger Ms2ger closed this as completed in c74c587 Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

5 participants