Port non-ISO calendar fixes from proposal-temporal PRs #1977 and #1983 #112
Conversation
This file contains hidden or 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
The current code is correct, so no need for this comment.
`year` might have been mutated in the first line of this method. So it's not OK to pass `calendarDate` to `maximumMonthLength`. Need to pass current year/month values instead.
This adds an additional check to make sure that the (hard-coded) era definitions for each calendar aren't missing required `isoEpoch` properties. Note that this code is only run once at load time, not each time calendars are accessed, so there's no runtime impact.
Re-order code to guarantee that `calendarDate.year` is set before calculating the number of months in that year. The old code would break for any calendar that a) used a constant `era` like `islamic` and b) had a variable number of months in each year like `hebrew`. Today no ICU calendars fit in that Venn diagram, but if any are added in the future this code will break when initializing any calendar using an eraYear/era pair in a property bag. Better to fix it now!
This was originally part of tc39/proposal-temporal#1977 but I removed it because it wasn't high-priority for JS. But it's still needed in this repo so I'm adding it back. Why? TS complains when you assign a maybe-undefined value to a must-not-be-undefined variable. This small runtime change is cleaner than adding a lotta type casts.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Port of tc39/proposal-temporal#1977 and tc39/proposal-temporal#1983 to fix a bunch of smaller issues with non-ISO calendar.
This is (hopefully!) the last ported PR so that #109 can be limited to type changes only.
We should merge this PR only after tc39/proposal-temporal#1977 is approved and merged.