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: Don't accept Z designators in strings for Plain types #1874

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Oct 15, 2021

Accepting this kind of string in Plain types poses a risk when
deserializing from databases, as some attach local-time-zone semantics to
this kind of string. This could cause user-facing off-by-one-day bugs.

(Note: this could probably be refactored to have these strings actually be invalid according to the ISO 8601 grammar, but this PR is set up so that could be done separately as an editorial change.)

Closes: #1751

@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #1874 (efe75c0) into main (fb69aeb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1874   +/-   ##
=======================================
  Coverage   95.01%   95.02%           
=======================================
  Files          19       19           
  Lines       10940    10949    +9     
  Branches     1732     1739    +7     
=======================================
+ Hits        10395    10404    +9     
  Misses        530      530           
  Partials       15       15           
Flag Coverage Δ
test262 81.66% <50.00%> (-0.13%) ⬇️
tests 89.84% <85.71%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 95.25% <100.00%> (+0.01%) ⬆️

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 fb69aeb...efe75c0. Read the comment docs.

@ptomato
Copy link
Collaborator Author

ptomato commented Oct 15, 2021

Draft until presented at TC39

@ptomato ptomato marked this pull request as draft October 15, 2021 00:19
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.

Looks great, thanks!

@ptomato ptomato added the spec-text Specification text involved label Oct 15, 2021
ptomato added a commit to ptomato/test262 that referenced this pull request Oct 19, 2021
(Needs more complete test coverage.)
Tests for the normative changes made to Temporal in
tc39/proposal-temporal#1874
ptomato added a commit to ptomato/test262 that referenced this pull request Oct 23, 2021
(Needs more complete test coverage.)
Tests for the normative changes made to Temporal in
tc39/proposal-temporal#1874
ptomato added a commit to ptomato/test262 that referenced this pull request Oct 26, 2021
(Needs more complete test coverage.)
Tests for the normative changes made to Temporal in
tc39/proposal-temporal#1874
@ptomato ptomato marked this pull request as ready for review October 29, 2021 19:52
@ptomato
Copy link
Collaborator Author

ptomato commented Oct 29, 2021

This gained consensus at the October TC39 plenary.

I am currently working on updating the tests.

ptomato added a commit to ptomato/test262 that referenced this pull request Nov 1, 2021
(Needs more complete test coverage.)
Tests for the normative changes made to Temporal in
tc39/proposal-temporal#1874
@ptomato ptomato force-pushed the 1751-plaindate-z-strings branch 2 times, most recently from 9556bca to cccb5a3 Compare November 18, 2021 22:11
@ptomato
Copy link
Collaborator Author

ptomato commented Nov 18, 2021

This change unearthed that we have some really out of date cookbook examples ☹️

Maybe we can get #1930 in first and then land this one.

Accepting this kind of string in Plain types poses a risk when
deserializing from databases, as some attach local-time-zone semantics to
this kind of string. This could cause user-facing off-by-one-day bugs.

Closes: #1751
@ptomato ptomato merged commit cd2dc7d into main Nov 24, 2021
@ptomato ptomato deleted the 1751-plaindate-z-strings branch November 24, 2021 01:42
justingrant added a commit to justingrant/proposal-temporal that referenced this pull request Nov 24, 2021
In tc39#1874, we removed the ability to parse Z strings with Plain types'
`from` methods. This PR explains this change and documents workarounds.

The docs for PlainYearMonth and PlainMonthDay (where Z strings are rare)
get only minimal text. PlainDate, PlainDateTime, and PlainTime (where
Z-string use cases are most likely) get a little more explanation and a
short code sample.
@justingrant
Copy link
Collaborator

Cool! I opened #1942 to describe these changes in the docs.

ptomato pushed a commit that referenced this pull request Nov 24, 2021
In #1874, we removed the ability to parse Z strings with Plain types'
`from` methods. This PR explains this change and documents workarounds.

The docs for PlainYearMonth and PlainMonthDay (where Z strings are rare)
get only minimal text. PlainDate, PlainDateTime, and PlainTime (where
Z-string use cases are most likely) get a little more explanation and a
short code sample.
12wrigja added a commit to js-temporal/temporal-polyfill that referenced this pull request Nov 30, 2021
ptomato added a commit that referenced this pull request Dec 3, 2021
Since #1874, strings with Z UTC designator are disallowed for Plain types.
This was not yet reflected in the manual ISO string generator test.
Ms2ger pushed a commit that referenced this pull request Dec 3, 2021
Since #1874, strings with Z UTC designator are disallowed for Plain types.
This was not yet reflected in the manual ISO string generator test.
ptomato added a commit to ptomato/test262 that referenced this pull request Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec-text Specification text involved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PlainDate.from('2020-01-01T00:00Z') is problematic
2 participants