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

Min/max values for all types #402

Merged
merged 6 commits into from
Feb 27, 2020
Merged

Min/max values for all types #402

merged 6 commits into from
Feb 27, 2020

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Feb 24, 2020

Closes: #24

@ptomato
Copy link
Collaborator Author

ptomato commented Feb 24, 2020

This includes a commit with a proposed resolution for #396, so we should make sure that is the consensus before merging this.

@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

Merging #402 into main will increase coverage by 0.36%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #402      +/-   ##
==========================================
+ Coverage   86.46%   86.82%   +0.36%     
==========================================
  Files          17       17              
  Lines        3450     3545      +95     
  Branches      399      427      +28     
==========================================
+ Hits         2983     3078      +95     
+ Misses        436      435       -1     
- Partials       31       32       +1     
Impacted Files Coverage Δ
polyfill/lib/date.mjs 78.82% <0.00%> (+2.03%) ⬆️

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 fad9716...c999bf4. Read the comment docs.

Copy link
Collaborator

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes seem fine, assuming we agree on the limits.

polyfill/lib/ecmascript.mjs Outdated Show resolved Hide resolved
@ptomato
Copy link
Collaborator Author

ptomato commented Feb 25, 2020

It looked like there was general agreement in #24 and #198 about the limits, though since this depends on the fix from #396 I would nonetheless not want to merge this yet. I'll merge the misc. bugfixes to master, but keep the limit commits in this pull request until #396 is resolved.

EDIT: Ah, never mind, it's not possible to merge only part of a PR without creating a new PR

This return value is always the same as what's passed in, so there's no
need for it.
Two pull requests crossed each other mid-flight.
Practically speaking the limits of Absolute were already the same as the
limits of old-style JavaScript Date, because we go through Date.UTC() to
get the number of milliseconds. This adds tests and fixes to get the
edge cases right.
- DateTime's limits are exactly enough to represent any Absolute in any
  valid TimeZone (offset -24 to +24 hours, exclusive at both
  boundaries.)
- Date's limits are exactly enough that a call of getDate() on any valid
  DateTime succeeds.
- YearMonth's limits are exactly enough that a call of getYearMonth() on
  any valid Date succeeds.

This also means that the inverse conversions of those mentioned above
can fail. If they are called in 'constrain' disambiguation mode, then
they'll be clamped to the nearest in-range value for the resulting type.
The 'balance' mode can't do anything in this case, so it'll behave like
'reject'.

Also ensures that plus() and minus() can't have results that are out of
the range.

See: #24.
Duration's lower limit is 0. It has no upper limit for any of its
fields, but they cannot be so large that their Number representation
becomes Infinity.

When serializing them, we format the numbers using BigInt.toString() in
order to avoid printing them in scientific notation, which would be an
invalid ISO 8601 string.
@ptomato
Copy link
Collaborator Author

ptomato commented Feb 27, 2020

Rebased on #396.

@ptomato ptomato merged commit ca41c94 into tc39:main Feb 27, 2020
@ptomato ptomato deleted the 24-years-range branch February 27, 2020 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TODO: Define min/max values
3 participants