-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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: customParseFormat strict mode bug when parsing with timezones (#929) #1467
base: dev
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1467 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 179 179
Lines 1996 2008 +12
Branches 507 513 +6
=========================================
+ Hits 1996 2008 +12 ☔ View full report in Codecov by Sentry. |
const currentOffset = this.utcOffset() | ||
if (isStrict) { | ||
if ( | ||
(parsedOffsetMilliseconds !== undefined && date.replace(/[+-]\d\d:?\d\d$/, '') !== this.subtract(currentOffset, 'minute').subtract(parsedOffsetMilliseconds, 'millisecond').format(format.replace(/(ZZ|ZZZ)$/, ''))) |
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.
Why is ZZZ
(triple zed) here? I don't see in the docs: https://day.js.org/docs/en/parse/string-format
Would you please add the relevant test, please? |
@iamkun I've added the missing tests. Totally forgot about that. |
Thanks for the PR, need some time to review this. |
Just a small concern, can we re-order the logic a little bit? Cause to me, it's a little complicated to understand the code of this PR. |
@iamkun I've tried to keep the order of the logic similar to how it was before this PR in order to make merging easier and keep the logic mostly in the same place as it was before. What kind of refactoring did you think about? |
Any news on this? |
Perhaps it would be a good idea to allow for all time zone formats that dayjs supports in line 253? |
@iamkun : the check for strictness (even in the original implementation) makes use of the 'format' function of dayjs. But the 'format' definition for the token 'Do' (Day of Month with ordinal) requires the AdvancedFormat plugin. Probably it would be a good idea to add a hint to the 'String + Format' documentation saying the using the 'Do' format requires the AdvancedFormat plugin to work. |
Hello @iamkun, are you willing to accept this PR? If not, what's missing? |
The
parseFormattedInput
function returns a UTC object for dates with timezones. When using dayjs().format() it usually formats the date with the current timezone. This fix manually removes the current timezone's offset and manually adds the originally parsed offset. Then the timezone part (+/-xx:xx or +-xxxx) will be removed from both strings and the timestamp without the timezone can be compared correctly.Resolves #929