Polyfill: Throw RangeError if there's an invalid offset string in ZonedDateTime-representing property bags
#1976
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## main #1976 +/- ##
=======================================
Coverage 94.96% 94.96%
=======================================
Files 19 19
Lines 10953 10962 +9
Branches 1740 1742 +2
=======================================
+ Hits 10401 10410 +9
Misses 535 535
Partials 17 17
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
justingrant
added
no-spec-text
justingrant
changed the title
offset string in ZonedDateTime-representing property bagsoffset string in ZonedDateTime-representing property bags
justingrant
added a commit
to justingrant/temporal-polyfill
that referenced
this pull request
Dec 13, 2021
This PR focuses on unblocking `strictNullChecks` and `strictPropertyInitialization` in ecmascript.ts and calendar.ts. It's intended as a complement to @jens-ox's work to remove `GetIntrinsic`. This PR: * Enables strictNullChecks: true and strictPropertyInitialization: true in tsconfig.json. * Improves calendar.ts TS types (this was most of the work in this PR) * Removes all unnecessary use of `any` throughout the polyfill * Updates types of ecmascript.ts and fixes a number of type bugs * Ports several proposal-temporal PRs: * tc39/proposal-temporal#1975 - Refactors to align ecmascript.mjs with spec text * tc39/proposal-temporal#1974 - Ensure that Temporal prototypes aren't writable * tc39/proposal-temporal#1976 - Throw RangeError if there's an invalid offset string in ZonedDateTime-representing property bags * Fixes a few index.d.ts types * Refactors a few types in intl.ts * A handful of trivial type changes in other files (ecmascript.ts and calendar-ts were 95%+ of the work). I'd like to split out the runtime behavior changes (mostly the ported PRs above) into a smaller PR which should make it easier to review changes that can actually break things at runtime. I also need to port tests over from tc39/proposal-temporal#1976. Once those are split out, this PR should be ready to review.
Ms2ger
reviewed
Dec 13, 2021
justingrant
added a commit
to justingrant/temporal-polyfill
that referenced
this pull request
Dec 13, 2021
This PR focuses on unblocking `strictNullChecks` and `strictPropertyInitialization` in ecmascript.ts and calendar.ts. It's intended as a complement to @jens-ox's work to remove `GetIntrinsic`. This PR: * Enables strictNullChecks: true and strictPropertyInitialization: true in tsconfig.json. * Improves calendar.ts TS types (this was most of the work in this PR) * Removes all unnecessary use of `any` throughout the polyfill * Updates types of ecmascript.ts and fixes a number of type bugs * Ports several proposal-temporal PRs: * tc39/proposal-temporal#1975 - Refactors to align ecmascript.mjs with spec text * tc39/proposal-temporal#1974 - Ensure that Temporal prototypes aren't writable * tc39/proposal-temporal#1976 - Throw RangeError if there's an invalid offset string in ZonedDateTime-representing property bags * Fixes a few index.d.ts types * Refactors a few types in intl.ts * A handful of trivial type changes in other files (ecmascript.ts and calendar-ts were 95%+ of the work). I'd like to split out the runtime behavior changes (mostly the ported PRs above) into a smaller PR which should make it easier to review changes that can actually break things at runtime. I also need to port tests over from tc39/proposal-temporal#1976. Once those are split out, this PR should be ready to review.
justingrant
added a commit
to justingrant/temporal-polyfill
that referenced
this pull request
Dec 13, 2021
This PR focuses on unblocking `strictNullChecks` and `strictPropertyInitialization` in ecmascript.ts and calendar.ts. It's intended as a complement to @jens-ox's work to remove `GetIntrinsic`. This PR: * Enables strictNullChecks: true and strictPropertyInitialization: true in tsconfig.json. * Improves calendar.ts TS types (this was most of the work in this PR) * Removes all unnecessary use of `any` throughout the polyfill * Updates types of ecmascript.ts and fixes a number of type bugs * Ports several proposal-temporal PRs: * tc39/proposal-temporal#1975 - Refactors to align ecmascript.mjs with spec text * tc39/proposal-temporal#1974 - Ensure that Temporal prototypes aren't writable * tc39/proposal-temporal#1976 - Throw RangeError if there's an invalid offset string in ZonedDateTime-representing property bags * tc39/proposal-temporal#1977 - Refactor and fix non-ISO calendars in polyfill * Fixes a few index.d.ts types * Refactors a few types in intl.ts * A handful of trivial type changes in other files (ecmascript.ts and calendar-ts were 95%+ of the work). I'd like to split out the runtime behavior changes (mostly the ported PRs above) into a smaller PR which should make it easier to review changes that can actually break things at runtime. I also need to port tests over from tc39/proposal-temporal#1976. Once those are split out, this PR should be ready to review.
justingrant
force-pushed
the
fix-ParseTimeZoneOffsetString-usage
branch
2 times, most recently
from
aac20ea
to
c54df25
Compare
Ensure that, per spec, a RangeError is thrown when an invalid offset is passed to ParseTimeZoneOffsetString. Note that validation of the offset in ISO strings is handled separately; this PR only fixes cases where an offset is parsed on its own, e.g. when property bag inputs are used in `ZonedDateTime#[with|from|equals|until|since|compare]` or as options in `Duration#[add|subtract|compare|round|total]`. The problem was also present in `TimeZone.from` and `ZonedDateTime#withTimeZone` but was caught downstream so didn't fail any previous tests. This commit also renames `ParseOffsetString` to `ParseTimeZoneOffsetString` which is the AO name in the spec. This commit also adds a new `ES.TestTimeZoneOffsetString` function which checks to see if the string matches the offset regex. This is used in cases where throwing on invalid offset strings is not desired.
justingrant
force-pushed
the
fix-ParseTimeZoneOffsetString-usage
branch
from
c54df25
to
c83a394
Compare
|
All review feedback is addressed, but the code has changed enough that I'd appreciate a fresh review. Thanks! |
ptomato
approved these changes
Dec 15, 2021
justingrant
added a commit
to justingrant/temporal-polyfill
that referenced
this pull request
Dec 15, 2021
justingrant
added a commit
to justingrant/temporal-polyfill
that referenced
this pull request
Dec 15, 2021
justingrant
added a commit
to js-temporal/temporal-polyfill
that referenced
this pull request
Dec 15, 2021
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.
Ensure that the correct RangeError is thrown when an invalid offset is passed to ParseTimeZoneOffsetString.
Note that validation of the offset in ISO strings is handled separately; this PR only fixes cases where an offset is parsed on its own, e.g. when property bag inputs are used in
ZonedDateTime#(with|from|equals|until|since|compare)or asrelativeTooptions passed toDuration#(add|subtract|compare|round|total). The problem was also present inTimeZone.fromandZonedDateTime#withTimeZonebut was caught downstream so didn't fail any previous tests.Fixes #1972.