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

Temporal edits #37905

Merged
merged 8 commits into from
Feb 4, 2025
Merged

Temporal edits #37905

merged 8 commits into from
Feb 4, 2025

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Feb 1, 2025

Description

A couple of minor edits to the Temporal docs: a few mislabeled or missing links, a clarification about ±HH:mm:ss.sssssssss offset parts of RFC 9557 strings, and another clarification about the representable range covered by Temporal objects.

Motivation

I was asked to pitch in to help review the original Temporal docs PR, but didn't have time. So I'm going over it now and fixing anything that comes to mind. This is the result of one session of doing that.

Additional details

Related issues and pull requests

Relates to #37344

If a Temporal object would represent a date outside of the supported
range, this always throws a RangeError, regardless of the value of the
overflow option.
@ptomato ptomato requested a review from a team as a code owner February 1, 2025 02:02
@ptomato ptomato requested review from Josh-Cena and removed request for a team February 1, 2025 02:02
@github-actions github-actions bot added Content:JS JavaScript docs size/s [PR only] 6-50 LoC changed labels Feb 1, 2025
@Josh-Cena
Copy link
Member

Thanks! Is this ready to review as-is, or do you plan to add more stuff to it?

Copy link
Contributor

github-actions bot commented Feb 1, 2025

Preview URLs (34 pages)

(comment last updated: 2025-02-04 01:34:01)

@ptomato
Copy link
Contributor Author

ptomato commented Feb 1, 2025

I figured I'd keep this easy to review and open a separate PR when I have another batch of edits, so this is ready to review. (If you'd prefer that I just make one PR instead, let me know and I'll do that.)

@github-actions github-actions bot added size/m [PR only] 51-500 LoC changed and removed size/s [PR only] 6-50 LoC changed labels Feb 1, 2025
@Josh-Cena
Copy link
Member

Weirdly, I can't push to your branch that I checked out with gh pr checkout, but I can push normally via the GH interface. I think it's because your repo is a fork of Eric's, not a fork of ours. Perhaps next time you would want to branch from a fork of ours instead, to avoid surprises like this.

@ptomato
Copy link
Contributor Author

ptomato commented Feb 3, 2025

Oh, thanks for the heads up. That must be a leftover from when I was reviewing Eric's work on this originally! I'll delete my fork and recreate it from the main MDN repo. (But I don't know if that will mess up this PR, so to be safe I'll only do that after this PR gets merged.) In the meantime, let me know if you'd like me to push anything to this branch.

@Josh-Cena
Copy link
Member

Josh-Cena commented Feb 3, 2025

I'm working on extracting all information about valid dates to a shared linkable section, and afterwards I'll merge it. The PR will be closed if the fork gets deleted so yes you should wait until it gets merged.

@Josh-Cena
Copy link
Member

@ptomato While researching the valid range of PlainYearMonth, I found some weird behaviors that I'm not sure the spec allows or not.

Temporal.PlainDate.from("+275760-09-13"); // works
Temporal.PlainDate.from("+275760-09-13").withCalendar("chinese"); // works
Temporal.PlainDate.from("+275760-09-13").withCalendar("chinese").year; // RangeError in both FF and the js-temporal polyfill
Temporal.PlainDate.from("+275760-09-13").withCalendar("chinese").toPlainYearMonth(); // RangeError

Do non-ISO calendars have smaller ranges of valid values compared to ISO 8601?

@ptomato
Copy link
Contributor Author

ptomato commented Feb 4, 2025

@Josh-Cena Good find! I looked into it and I believe this behaviour is not allowed by the spec. Any valid PlainDate object should be guaranteed to be valid in all calendars, and therefore have a valid year property. The toPlainYearMonth() method of a valid PlainDate might throw if the first day of the calendar month is outside of the valid PlainYearMonth range (ISO -271821-04 to +275760-09) but I believe that can only happen with the minimum PlainYearMonth, not the maximum.

I've looked at why this happens in the js-temporal polyfill and it's because we don't have a proper calendar implementation, we just fake it using iterative guesses from Intl.DateTimeFormat, and there's a point where we try to query the 1st day of the following year which of course fails in this case. So that's a bug. However, that doesn't explain why this fails on Firefox, maybe a bug in ICU4X?

@Josh-Cena
Copy link
Member

I've updated this PR with my understanding of valid date ranges. Could you look if it makes sense? (Modulo my question above, which may or may not be worth including in the docs).

@Josh-Cena
Copy link
Member

So how does this look?

@ptomato
Copy link
Contributor Author

ptomato commented Feb 4, 2025

@Josh-Cena Looks great, ship it! 😄

@Josh-Cena Josh-Cena merged commit 3cecb79 into mdn:main Feb 4, 2025
8 checks passed
@ptomato ptomato deleted the temporal-edits branch February 4, 2025 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:JS JavaScript docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants