Repository navigation
Conversation
ptomato
added
the
run snapshot tests
ptomato
force-pushed
the
sync-ecma262-editor-updates-to-polyfill
branch
from
October 22, 2026 20:26
06d012c to
caeea69
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3333 +/- ##
==========================================
+ Coverage 98.39% 98.41% +0.01%
==========================================
Files 22 22
Lines 10796 10883 +87
Branches 1875 1930 +55
==========================================
+ Hits 10623 10710 +87
Misses 161 161
Partials 12 12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ptomato
force-pushed
the
sync-ecma262-editor-updates-to-polyfill
branch
from
November 10, 2026 00:52
caeea69 to
d94c864
Compare
Member
Author
|
I removed a few of the commits that I originally included in this PR. I need to look at them some more. |
This is in order to validate the spec refactor
(but upper-snake-cased)
Keep functions IsCalendarUnit, IsDateUnit, IsTimeUnit for "is a (calendar/date/time) unit" respectively.
Also do some of ECMA-262's renames on the "ns1" and "ns2" parameters and DifferenceInstant → DifferenceEpochNanoseconds.
The GetUTCEpochMilliseconds operation is not in the spec, but the spec's new ISODateToEpochDays algorithm has comparable performance to the optimized GetUTCEpochMilliseconds we already had, so we can just use that. We can also use it in GetUTCEpochNanoseconds directly, rather than delegating to GetUTCEpochMilliseconds.
These assertions would previously always pass, because the value being compared to undefined was always a number (should have been compared to NaN.) That was a harmless bug but also made the assertions useless. Not a spec bug.
Adjust expectations to cover expected failures in new tests, and new passes that have been pulled in with a new Node.js version.
ptomato
force-pushed
the
sync-ecma262-editor-updates-to-polyfill
branch
from
November 10, 2026 01:09
d94c864 to
55574ee
Compare
Ms2ger
approved these changes
Nov 10, 2026
Comment on lines
+201
to
+202
| if (nonIntHandling === 'truncate') mv = MathTrunc(mv); | ||
| if (!IsIntegralNumber(mv)) throw new RangeErrorCtor(`'${property}' must be an integer, not ${arg}`); |
Collaborator
There was a problem hiding this comment.
This is written somewhat confusingly. I don't think there's edge cases where Math.trunc returns a non-integral number? But it matches the spec, so fine to keep it
Comment on lines
-2827
to
+2824
| // In the spec, IsValidEpochNanoseconds returns a boolean and call sites are | ||
| // responsible for throwing. In the polyfill, ValidateEpochNanoseconds takes its | ||
| // place so that we can DRY the throwing code. | ||
| // In the spec, IsWithinEpochNanosecondsInterval returns a boolean and call | ||
| // sites are responsible for throwing. In the polyfill, ValidateEpochNanoseconds | ||
| // takes its place so that we can DRY the throwing code. |
Collaborator
There was a problem hiding this comment.
Nit: please split this change into a separate commit rather than the CombineISODateAndTimeRecord one
Comment on lines
+2793
to
+2794
| if (microsecond < 0 || microsecond > 999) return false; | ||
| return nanosecond >= 0 && nanosecond <= 999; |
Collaborator
There was a problem hiding this comment.
I'm not generally a fan of if-return true-else-return false, but I think it'd look better here if the nanosecond check looked the same as the others
| const NS_PER_TIME_UNIT = new MapCtor( | ||
| Call(ArrayPrototypeFlatMap, TEMPORAL_UNITS, [([, s, , l]) => (l ? [[s, l]] : [])]) | ||
| ); | ||
| const NS_PER_TIME_UNIT = new MapCtor(Call(ArrayPrototypeFlatMap, TEMPORAL_UNITS, [([, s, l]) => (l ? [[s, l]] : [])])); |
Collaborator
There was a problem hiding this comment.
Unrelated to the change, but this feels like a submission to an obfuscated code contest
| }, | ||
| [] | ||
| ]); | ||
| const ALLOWED_UNITS = Call( |
Collaborator
There was a problem hiding this comment.
Nit (to be changed separately): I don't think this should be in upper snake case
Comment on lines
-756
to
+769
| return GetOption(options, 'overflow', ['constrain', 'reject'], 'constrain'); | ||
| const value = options.overflow; | ||
| if (value === undefined) return 'constrain'; | ||
| const stringValue = ToString(value); | ||
| validateStringValue('overflow', stringValue, ['constrain', 'reject']); | ||
| return stringValue; |
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
These commits are refactors in the reference polyfill, which match refactors to the spec requested in round 1 of the editors review in tc39/ecma262#3759. I used this code to verify the spec refactors as I made them, and these edits have been sitting in my local git checkout for a couple of months; they probably belong here, rather than being lost.