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

fix(material/datepicker): fix handling of short years #20709

Merged
merged 1 commit into from Oct 4, 2020

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented Oct 2, 2020

This should hopefully fix test flakiness introduced by dd49fa6. The previous fix seems to sometimes report an incorrect date based on the time zone the test runs in

@mmalerba mmalerba added P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release labels Oct 2, 2020
@mmalerba mmalerba requested a review from crisbeto October 2, 2020 23:08
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 2, 2020
@mmalerba mmalerba force-pushed the short-year branch 2 times, most recently from 39ddaf5 to 7066d31 Compare October 2, 2020 23:30
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

I think that this is basically the same as the old fix, except that the previous approach had the small optimization that we only mutated the new date if we detect the edge case for years 0 to 99, whereas this approach does it every time. My concern is that since we're setting the hours/minutes/seconds to 0, we might break some people that depended on the old behavior.

If we only wanted to fix the tests, we could create the date as February 1st, instead of January 1st.

@mmalerba
Copy link
Contributor Author

mmalerba commented Oct 3, 2020

I think the old behavior set the hour/m/s/ms to 0 anyways? If you construct a date without specifying those 0 is assumed. I think this does differ from the old way. The old way you overwrote only the year, and my theory is that for locales where the timezone offset has changed over time this was leading to dates that were slightly off. At the very least this seems to pass on CI where the old approach was regularly failing

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

My hesitation comes from the fact this is used anywhere we interact with a date, not just when it's formatted which could break people in subtle ways, whereas the previous fix only affected formatting. Let's see how it goes during the presubmit and we can adjust the test, instead of changing the logic, if it ends up breaking too many targets.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Oct 4, 2020
@mmalerba
Copy link
Contributor Author

mmalerba commented Oct 4, 2020

I still don't quite understand your concern. I modified the same 2 methods your change did: _createDateWithOverflow and _format. Yes, _createDateWithOverflow zeros out the h, m, s, ms, but it always did that, so that's nothing new

@crisbeto
Copy link
Member

crisbeto commented Oct 4, 2020

I think that the changes are fine, but at a glance it isn't obvious why they would be fixing the test, considering that they're setting the same values as the defaults.

Looking through it again, I have a theory about what might've been throwing it off. Note how _createDateWithOverflow creates a regular date and _format creates a UTC one, however when both of them go through _correctYear, we call setFullYear no matter whether it's UTC or not. Maybe the problem is that _format should call setUTCFullYear and _createDateWithOverflow should use setFullYear (which is basically what your changes do after you remove _correctYear)?

@mmalerba mmalerba merged commit b3f1fb3 into angular:master Oct 4, 2020
mmalerba added a commit that referenced this pull request Oct 4, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants