-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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($shared-utils): fix date parse logic for permalinks #2181
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, where did you meet this problem? Could you provide a repro?
Since I tested the function and saw the behavior of new Date, but I also found that our current code (with getMonth and getDate) somehow work fine.
It might be difficult for you to validate the issue without resetting your clock since you might not have too much delta between your local timezone and UTC. The Mozilla website from the link above also says...
|
Well, my local timezone is UTC+8, so here is what I met: const date = new Date("2020-1-1"); // I got: 2019-12-31T16:00:00.000Z
const month = date.getMonth() + 1; // I got: 1
const day = date.getDate(); // I got: 1 Obviously, the date is inconsistent as you said. However, something like That's why I wish you can link me where you met the problem. |
Oh, I got ya. I thought you were asking which branch had the issue. I don't have anything pushed in Github to demo, but I was getting the issue from version I'm surprised Here are my results: const d1 = new Date('2020-01-01'); // Tue Dec 31 2019 19:00:00 GMT-0500 (Eastern Standard Time)
const d2 = new Date('2020-1-1'); // Wed Jan 01 2020 00:00:00 GMT-0500 (Eastern Standard Time)
d1.getMonth() + 1; // 12
d1.getDate(); // 31
d2.getMonth() + 1; // 1
d2.getDate(); // 1 The effect was that, though my |
Hey, what should we do about this PR? @enagic if we're not able to reproduce we'll close this PR |
And here I thought, "Well it works fine in my local environment" wasn't a valid QA response! But in all seriousness, I suspect the first step to reduce should be to reset your local clock to a different timezone. In my case, it's EST. |
I can indeed reproduce it after I set my timezone to EST: repo |
This addresses the way the date is parsed, which, when it's a string is causing the input for the Date constructor to assume the date to be in ISO 8601 format (UTC), and when it's a Date, is always UTC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. At least one more 'Approve', it'll be merge.
Do we still want to have this merged? @newsbielt703, @ulivz, @kefranabg |
This addresses the way the date string is parsed, which is causing the input for the Date constructor to assume the date to be in ISO 8601 format (UTC). This results in the generated permalink to be one day off (depending on your local timezone).
Example:
new Date('2020-01-01')
results in the date being parsed asTuesday, December 31, 2019 19:00:00 GMT-0500
for EST.new Date('2020-1-01')
results in the date being parsed asWednesday, January 01, 2020 00:00:00 GMT-0500
for EST.As noted in developer.mozilla.org
Summary
What kind of change does this PR introduce? (check at least one)
If changing the UI of default theme, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
This will change the logic used for existing permalinks, possibly offsetting the published date by 1 day.
The PR fulfills these requirements:
fix #xxx[,#xxx]
, where "xxx" is the issue number)You have tested in the following browsers: (Providing a detailed version will be better.)
If adding a new feature, the PR's description includes:
To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.
Other information: