Skip to content

Conversation

@ptomato
Copy link
Collaborator

@ptomato ptomato commented Aug 20, 2021

Previously we had decided to always throw on these strings. Based on
feedback from the IETF SEDATE working group, we are going to allow them.
This means changes to what kinds of strings are allowed in several
contexts. See #1695 for more information.

The following diagram may be helpful when reading this commit.

"datetime" = e.g. "2021-08-19T17:30"
"offset" = e.g. "-07:00"
"bracket" = e.g. "[America/Vancouver]", "[-07:00]" or "[UTC]"

TimeZone.from:

  • datetime + Z -> UTC
  • datetime + offset -> offset time zone
  • datetime + bracket -> IANA time zone
  • datetime + Z + bracket -> "
  • datetime + offset + bracket -> "
  • throws on a bare datetime string

ZonedDateTime.from:

  • datetime + bracket -> preserve wall time in the IANA time zone
  • datetime + Z + bracket -> preserve exact time in the IANA time zone
  • datetime + offset + bracket -> consult offset option if ambiguous
  • throws on a string with no bracket

Instant.from:

  • datetime + Z -> preserve exact time
  • datetime + offset -> "
  • datetime + Z + bracket -> preserve exact time, ignore bracket
  • datetime + offset + bracket -> "
  • throws on a bare datetime string, or datetime + bracket

relativeTo:

  • datetime + bracket -> do what ZonedDateTime.from does
  • datetime + Z + bracket -> "
  • datetime + offset + bracket -> "
  • anything else -> do what PlainDateTime.from does

Closes: #1695
Closes: #1696

@ptomato
Copy link
Collaborator Author

ptomato commented Aug 20, 2021

Tests to follow.

@ptomato
Copy link
Collaborator Author

ptomato commented Aug 20, 2021

(Also, marking as draft because normative)

@ptomato ptomato marked this pull request as draft August 20, 2021 00:48
@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #1749 (71eff06) into main (e780f7c) will increase coverage by 0.06%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##             main    #1749      +/-   ##
==========================================
+ Coverage   94.85%   94.91%   +0.06%     
==========================================
  Files          19       19              
  Lines       10882    10807      -75     
  Branches     1729     1736       +7     
==========================================
- Hits        10322    10258      -64     
+ Misses        547      536      -11     
  Partials       13       13              
Flag Coverage Δ
test262 74.87% <96.87%> (-1.48%) ⬇️
tests 90.14% <93.33%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 94.81% <100.00%> (-0.04%) ⬇️
polyfill/lib/zoneddatetime.mjs 98.39% <100.00%> (-0.07%) ⬇️
polyfill/lib/plainmonthday.mjs 93.19% <0.00%> (-0.69%) ⬇️
polyfill/lib/instant.mjs 94.44% <0.00%> (+0.34%) ⬆️
polyfill/lib/plaintime.mjs 90.74% <0.00%> (+1.92%) ⬆️

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 e780f7c...9a2bf1d. Read the comment docs.

Copy link
Collaborator

@justingrant justingrant left a comment

Choose a reason for hiding this comment

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

LGTM

@ptomato ptomato marked this pull request as ready for review August 31, 2021 18:14
@ptomato
Copy link
Collaborator Author

ptomato commented Aug 31, 2021

This change achieved consensus in TC39. @Ms2ger Do you want to take a final look at the tests I added recently?

Previously we had decided to always throw on these strings. Based on
feedback from the IETF SEDATE working group, we are going to allow them.
This means changes to what kinds of strings are allowed in several
contexts. See #1695 for more information.

The following diagram may be helpful when reading this commit.

"datetime" = e.g. "2021-08-19T17:30"
"offset" = e.g. "-07:00"
"bracket" = e.g. "[America/Vancouver]", "[-07:00]" or "[UTC]"

TimeZone.from:
  - datetime + Z -> UTC
  - datetime + offset -> offset time zone
  - datetime + bracket -> IANA time zone
  - datetime + Z + bracket -> "
  - datetime + offset + bracket -> "
  - throws on a bare datetime string

ZonedDateTime.from:
  - datetime + bracket -> preserve wall time in the IANA time zone
  - datetime + Z + bracket -> preserve exact time in the IANA time zone
  - datetime + offset + bracket -> consult offset option if ambiguous
  - throws on a string with no bracket

Instant.from:
  - datetime + Z -> preserve exact time
  - datetime + offset -> "
  - datetime + Z + bracket -> preserve exact time, ignore bracket
  - datetime + offset + bracket -> "
  - throws on a bare datetime string, or datetime + bracket

relativeTo:
  - datetime + bracket -> do what ZonedDateTime.from does
  - datetime + Z + bracket -> "
  - datetime + offset + bracket -> "
  - anything else -> do what PlainDateTime.from does

Closes: #1695
Closes: #1696
@Ms2ger Ms2ger enabled auto-merge (rebase) September 2, 2021 12:58
@Ms2ger Ms2ger merged commit f6ac475 into main Sep 2, 2021
@Ms2ger Ms2ger deleted the 1695-z-bracket branch September 2, 2021 13:27
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.

TimeZone.from with ISO strings that have both Z and IANA name ZonedDateTime.from when the offset is "Z" (and an IANA name is present)

5 participants