fix(common): do not round up fractions of a millisecond in DatePipe - #38009
Closed
ajitsinghkaler wants to merge 1 commit into
Closed
ajitsinghkaler wants to merge 1 commit into
ajitsinghkaler wants to merge 1 commit into
Conversation
Contributor
Author
|
Test failures seem unrelated |
petebacondarwin
suggested changes
Jul 11, 2020
Contributor
There was a problem hiding this comment.
Thanks for putting this PR together @ajitsinghkaler. Can we tweak the commit message a bit.
Fixeslines should come beforeBREAKING CHANGEblocks. Otherwise they become part of the breaking change notice.- We should refer to
DatePiperather thandate pipesince that is the name of the class. - BREAKING CHANGE notices should describe what has changed, what type of application code could be affected, and, if possible, how to update your code to cope if you are affected.
My suggestion for the commit message would be:
fix(common): do not round up fractions of a millisecond in `DatePipe`
Currently, the `DatePipe` (via `formatDate()` rounds fractions of a millisecond to the
nearest millisecond. This can cause dates that are less than a millisecond before midnight
to be incremented to the following day.
The [ECMAScript specification](https://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.11)
defines that `DateTime` milliseconds should always be rounded down, so that `999.9ms`
becomes `999ms`.
This change brings `formatDate()` and so `DatePipe` inline with the ECMAScript
specification.
Fixes #37989
BREAKING CHANGE:
When passing a date-time formatted string to the `DatePipe` in a format that contains
fractions of a millisecond, the milliseconds will now always be rounded down rather than
to the nearest millisecond.
Most applications will not be affected by this change. If this is not the desired behaviour
then consider pre-processing the string to round the millisecond part before passing
it to the `DatePipe`.
ajitsinghkaler
force-pushed
the
milli-date
branch
from
July 12, 2020 02:41
e75d2a2 to
429498f
Compare
DatePipe
petebacondarwin
approved these changes
Jul 12, 2020
petebacondarwin
left a comment
Contributor
There was a problem hiding this comment.
Great! Just a tiny typo in the commit message... a missing closing bracket.
ajitsinghkaler
force-pushed
the
milli-date
branch
from
July 13, 2020 03:09
09a5d3f to
c8d0536
Compare
AndrewKushnir
approved these changes
Jul 13, 2020
Contributor
There was a problem hiding this comment.
Thanks for updating the comment in the code 👍
Since this is a breaking change, I've also started a global presubmit in Google's codebase and will share results as soon as I have them.
FYI, I've also added the "blocked" label for now, since we can merge this PR only when master branch becomes available for the changes related to v11 (since this is a breaking change).
Thank you.
Contributor
|
FYI, the presubmit went well, so we should be able to land it in v11. We would need to rebase and run another global presubmit before merging (once master becomes available for v11 changes). Thank you. |
Contributor
|
|
Currently, the `DatePipe` (via `formatDate()`) rounds fractions of a millisecond to the nearest millisecond. This can cause dates that are less than a millisecond before midnight to be incremented to the following day. The [ECMAScript specification](https://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.11) defines that `DateTime` milliseconds should always be rounded down, so that `999.9ms` becomes `999ms`. This change brings `formatDate()` and so `DatePipe` inline with the ECMAScript specification. Fixes angular#37989 BREAKING CHANGE: When passing a date-time formatted string to the `DatePipe` in a format that contains fractions of a millisecond, the milliseconds will now always be rounded down rather than to the nearest millisecond. Most applications will not be affected by this change. If this is not the desired behaviour then consider pre-processing the string to round the millisecond part before passing it to the `DatePipe`.
ajitsinghkaler
force-pushed
the
milli-date
branch
from
September 8, 2020 04:06
c8d0536 to
dffb4f6
Compare
Contributor
Author
|
@petebacondarwin rebased it |
Contributor
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
Date pipe rounds up milliseconds when passed fractional dates.
ECMAScript spec defines that non-integer values passed for any of the parameters to
new Date(year, month, date, hours, minutes, seconds, ms)
should call floor() to remove the fractional part.
BREAKING CHANGE: Some people may rely on the rounding functionality of date pipe
Fixes #37989
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Date pipe rounds fractional part of seconds
Issue Number: #37989
What is the new behavior?
Datepipe floors fractional seconds
Does this PR introduce a breaking change?
Other information