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

Consistency between string and objects args in plainDate.from() #2840

Open
jasonwilliams opened this issue May 15, 2024 · 8 comments
Open

Consistency between string and objects args in plainDate.from() #2840

jasonwilliams opened this issue May 15, 2024 · 8 comments

Comments

@jasonwilliams
Copy link
Member

jasonwilliams commented May 15, 2024

Some feedback I've received and noticed myself is the inconsistent handling of strings and objects in the plainDate.from() method. I know we have discussed this in the past and the documentation does specify this behavior but I would like some clarity on it (as we've made many changes since then).

A string is checked for a valid date and can throw if the date is not valid, where as an object will constrain the value. Here is an example:

const date = Temporal.PlainDate.from({ year: 2024, month: 2, day: 30 }); 
const date2 = Temporal.PlainDate.from('2024-02-30');

In this example date2 will throw due to the date being invalid. (This is rejected as the string is parsed)

My understanding is the rational for this came from string potentially coming in from an external source and less likely to be checked, thus the validation. However objects are more likely to be constructed by the developer and could be checked already.

I don't think this is a strong argument though. A developer could assume their input is being validated and one day switch to constructing an object (not realising the object is no longer being validated).

Do we need to ignore overflow for strings?

There's the opposite case where a developer may want to migrate from passing in objects to strings but still want the validation "turned off." This currently isn't possible as the overflow property is ignored

const date = Temporal.PlainDate.from('2024-02-30', { overflow: 'constrain'});

This will still throw. Currently we throw at string parsing so the overflow option isn't considered. Will this be expected behavior from developers, should we allow opting out of this like you can with objects?

Questions to answer

  • Can we offer the same default behavior to strings and objects?
  • Can we allow string arguments to override the default behavior (same as object)?
  • Are we happy with "constrain" being the default over "reject"
@ptomato
Copy link
Collaborator

ptomato commented May 15, 2024

My opinion: strings are different from property bags, because ISO 8601 and RFCs 3339 and 9557 define what's an acceptable string and what isn't. So far, we've always said that ISO strings should not be treated as if they can be "assembled" from parts, because that's what the property bag is for.

Constraining an invalid ISO string like 2024-02-30 to 2024-02-29 would be possible, but deciding how far we would have to stretch the ISO string grammar to include invalid strings just for the purposes of constraining them, would be tricky. Should 2024-01-32 be allowed? 2024-01-99? 2024-01-100?

With the larger issue I agree, overflow not working on strings is a weird corner of the API. I'd be open to finding a solution to make it less weird, but I'd want to do that in a web-compatible way, in a follow up proposal.

@nekevss
Copy link

nekevss commented May 16, 2024

Not sure it's needed, but I'd like to second the above on strings being different from property bags. An "overflowed" string doesn't exist. It's just an invalid date/time string to specification vs. the property bag, which is ambiguous.

A fun answer to the probably rhetorical question above. In theory, it would be 2024-01-99 cause you'd probably want to restrict the grammar on day to DAY = 2DIGIT. But to that logic, then 9999-99-99 should be a valid date string target.

@justingrant
Copy link
Collaborator

One other point that's probably captured in the links Philip posted above, but maybe worth calling out here: there's a pretty useful case for specifying an abnormally large month or day, which is to ergonomically get the last day of a month.

Temporal.PlainDate.from({ year: 2024, month: 2, day: 1000 });
// => 2024-02-29

The alternatives are much more verbose, e.g. get the first day of the month, add one month, and subtract one day.

IIRC, the existence of this ability was used as a justification for not adding an endOfMonth method. Given what we now know about the cost of public methods, we'll likely never add this method anyways, so having an alternative is good.

Interestingly, I noticed that this throws an exception:

Temporal.PlainDate.from({ year: 2024, month: 2, day: Infinity });
// => RangeError: invalid number value

This is expected, right? I seem to remember that at one point it worked, but I may be imagining things.

With the larger issue I agree, overflow not working on strings is a weird corner of the API. I'd be open to finding a solution to make it less weird, but I'd want to do that in a web-compatible way, in a follow up proposal.

I'm not sure this solution can exist without allowing invalid RFC 9557 strings (which I don't think we want to for exactly the arguments above).

The only reasonable change I could see us making here would be to change default behavior for objects to 'reject' to match the string behavior.

@sffc
Copy link
Collaborator

sffc commented May 16, 2024

The ISO string throws because it is an invalid ISO string. The constructor also throws as it is an invalid ISO date.

Temporal.PlainDate.from('2024-02-30') // throws
new Temporal.PlainDate(2024, 2, 30) // throws

The bag of fields does not throw because it uses the calendar protocol, which by default uses a constrain behavior as a best effort to construct a calendared date from the fields. It is just a thin wrapper over dateFromFields.

@acutmore
Copy link

Personally I was more surprised that the object bag form was not strict by default. I'm happy that the string form is strict.

The alternatives are much more verbose, e.g. get the first day of the month, add one month, and subtract one day.
IIRC, the existence of this ability was used as a justification for not adding an endOfMonth method. Given what we now know about the cost of public methods, we'll likely never add this method anyways, so having an alternative is good.

This doesn't seem too verbose (if the default was strict)?

Temporal.PlainDate.from({ year: 2024, month: 2, day: 1000 }, { overflow: 'constrain'});

@khawarizmus
Copy link
Contributor

IIRC, the existence of this ability was used as a justification for not adding an endOfMonth method. Given what we now know about the cost of public methods, we'll likely never add this method anyways, so having an alternative is good.

@justingrant Temporal won't have endOfMonth property?

@justingrant
Copy link
Collaborator

Correct. Because there's a relatively easy workaround (as detailed in this thread), we did not add an endOfMonth method. Furthermore, as we've subsequently learned about the code size constraints of ECMAScript engines (especially on Android and Apple Watch), it's very unlikely that we'll see convenience methods like endOfMonth() be added in the future.

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

No branches or pull requests

7 participants