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

Handle number-less DateTimeFormat Hebrew month #2034

Merged
merged 1 commit into from
Feb 4, 2022

Conversation

justingrant
Copy link
Collaborator

The ICU formats for the Hebrew calendar no longer output a month number, only the name. This PR works around this change. See https://bugzilla.mozilla.org/show_bug.cgi?id=1751833 for more context.

Fixes #2015.

The ICU formats for the Hebrew calendar no longer output a month
number, only the name. This commit works around this change. See
https://bugzilla.mozilla.org/show_bug.cgi?id=1751833.

Fixes tc39#2015.
@justingrant justingrant added no-spec-text PR can be ignored by implementors non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! labels Feb 4, 2022
@codecov
Copy link

codecov bot commented Feb 4, 2022

Codecov Report

Merging #2034 (1b1657e) into main (ad37d6e) will decrease coverage by 0.01%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2034      +/-   ##
==========================================
- Coverage   94.44%   94.43%   -0.02%     
==========================================
  Files          19       19              
  Lines       10989    11002      +13     
  Branches     1596     1597       +1     
==========================================
+ Hits        10379    10390      +11     
- Misses        590      592       +2     
  Partials       20       20              
Flag Coverage Δ
test262 81.15% <25.00%> (-0.04%) ⬇️
tests 88.73% <75.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
polyfill/lib/calendar.mjs 94.25% <87.50%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad37d6e...1b1657e. Read the comment docs.

@Ms2ger
Copy link
Collaborator

Ms2ger commented Feb 4, 2022

Can we test this?

@justingrant
Copy link
Collaborator Author

Can we test this?

If Node's ICU data is updated to include the problematic change (the same one that Chrome and FF have already picked up) then any property access using the Hebrew calendar will trigger this bug. So it will be tested by CI Demitasse tests (in intl.mjs) as soon as CI is updated to use a version of Node with the latest ICU data.

In the meantime, I manually verified it in latest Chrome and latest FF and Chrome. Before the fix in this PR, the following code will throw. After the fix, it won't.

Temporal.PlainDate.from('2020-01-01[u-ca=hebrew]').day;

Your comment made me realize that Node now has v17, so we should probably be testing it in CI. I'll open a new issue for that.

@Ms2ger Ms2ger merged commit 8b6fad5 into tc39:main Feb 4, 2022
@justingrant justingrant mentioned this pull request Feb 4, 2022
@justingrant
Copy link
Collaborator Author

BTW, I confirmed that the current (pre-PR) code fails under Node 17 tests. See https://github.com/tc39/proposal-temporal/runs/5069846827?check_suite_focus=true

Then I rebased the Node 17 PR (#2039) on top of this PR and verified that Node 17 passes.

justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Feb 4, 2022
justingrant added a commit to js-temporal/temporal-polyfill that referenced this pull request Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-spec-text PR can be ignored by implementors non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hebrew calendar documentation example broken by 'numeric' month option being ignored by Intl.DateTimeFormat
2 participants