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

Normative: Optimize calls into user code in InterpretISODateTimeOffset #1688

Merged
merged 1 commit into from Sep 2, 2021

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Aug 5, 2021

In the current spec text algorithm, there was an unnecessary call to
timeZone.getPossibleInstantsFor(), which potentially calls observably into
user code. Fix the algorithm so it doesn't make any more calls than
necessary.

@ptomato ptomato added the needs plenary input Needs to be presented to the committee and feedback incorporated label Aug 5, 2021
@ptomato ptomato requested a review from Ms2ger August 5, 2021 00:08
@codecov
Copy link

codecov bot commented Aug 5, 2021

Codecov Report

Merging #1688 (b994cf5) into main (fa9d547) will increase coverage by 1.00%.
The diff coverage is 100.00%.

❗ Current head b994cf5 differs from pull request most recent head 4409765. Consider uploading reports for the commit 4409765 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1688      +/-   ##
==========================================
+ Coverage   90.09%   91.10%   +1.00%     
==========================================
  Files          17       17              
  Lines       10896    10777     -119     
  Branches     1617     1608       -9     
==========================================
+ Hits         9817     9818       +1     
+ Misses       1041      945      -96     
+ Partials       38       14      -24     
Flag Coverage Δ
tests 90.15% <100.00%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 94.52% <100.00%> (+1.72%) ⬆️
polyfill/lib/zoneddatetime.mjs 90.62% <0.00%> (-0.44%) ⬇️
polyfill/lib/plainmonthday.mjs 83.56% <0.00%> (-0.23%) ⬇️
polyfill/lib/plaindate.mjs 72.06% <0.00%> (-0.07%) ⬇️
polyfill/lib/plainyearmonth.mjs 89.77% <0.00%> (-0.06%) ⬇️
polyfill/lib/plaindatetime.mjs 90.34% <0.00%> (-0.02%) ⬇️
polyfill/lib/duration.mjs 93.65% <0.00%> (+0.19%) ⬆️
polyfill/lib/instant.mjs 87.15% <0.00%> (+0.69%) ⬆️
polyfill/lib/calendar.mjs 91.40% <0.00%> (+1.41%) ⬆️
polyfill/lib/plaintime.mjs 79.72% <0.00%> (+1.68%) ⬆️
... and 1 more

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 fa9d547...4409765. Read the comment docs.

@ptomato
Copy link
Collaborator Author

ptomato commented Aug 7, 2021

Marking as draft so that the normative change isn't merged accidentally.

@ptomato ptomato marked this pull request as draft August 7, 2021 00:17
@ptomato ptomato marked this pull request as ready for review August 31, 2021 18:57
@ptomato
Copy link
Collaborator Author

ptomato commented Aug 31, 2021

This change achieved consensus in TC39.

@Ms2ger Would you be able to take a final look?

@Ms2ger Ms2ger force-pushed the optimize-interpretisodatetimeoffset branch from d4b1539 to 50d5af8 Compare September 2, 2021 13:44
In the current spec text algorithm, there was an unnecessary call to
timeZone.getPossibleInstantsFor(), which potentially calls observably into
user code. Fix the algorithm so it doesn't make any more calls than
necessary.
@Ms2ger Ms2ger force-pushed the optimize-interpretisodatetimeoffset branch from 50d5af8 to 4409765 Compare September 2, 2021 13:48
@Ms2ger Ms2ger enabled auto-merge (rebase) September 2, 2021 13:48
@Ms2ger Ms2ger merged commit b53fc25 into main Sep 2, 2021
@Ms2ger Ms2ger deleted the optimize-interpretisodatetimeoffset branch September 2, 2021 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs plenary input Needs to be presented to the committee and feedback incorporated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants