Fix issue #1596 - Provide way to specify multiple formats when creating a date in UTC #1914
Conversation
|
Perhaps we should add a list of the allowed separator characters ( |
|
And when we are at it, perhaps we should also add a list of the deviations from
|
|
I finally found the "older" pr #1467 that solves at least the issue with the offset in strict mode, when the format contains a
So now you have the choice :-) @iamkun by now there are a lot of open issues and quite some pr. Is there a way I could help you to reduce the number of open issues and pr? |
Codecov Report
@@ Coverage Diff @@
## dev #1914 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 181 184 +3
Lines 2074 2158 +84
Branches 544 579 +35
=========================================
+ Hits 2074 2158 +84
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
thanks, please allow me some time to go through this cool PR |
|
You've got all the time in the world; if my documentation of the changes needs improvement, feel free to ask. |
| } | ||
|
|
||
| // eslint-disable-next-line import/prefer-default-export | ||
| export const daysInMonth = (year, month) => { |
There was a problem hiding this comment.
we can use the built-in API
dayjs(year + '-' + month).daysInMonth()
There was a problem hiding this comment.
sorry for the late reply, but somehow I missed this comment :-(
Of course we could use dayjs(year + '-' + month).daysInMonth() instead of my 'auxiliary' method.
But the problem is that we do not have the dayjs object available to the checkOverflow() function.
Therefore I created the daysInMonth method in the customParseFormat/utils file.
As an additional bonus, this function runs faster than creating a new dayjs object (to be exact, more than 1 instance is created for getting daysInMonth).
What do you think about this?
## [1.11.5](iamkun/dayjs@v1.11.4...v1.11.5) (2022-08-12) ### Bug Fixes * ordinal for nl not working ([iamkun#2011](iamkun#2011)) ([c93c85e](iamkun@c93c85e)) * wrong ordinal for french locale ([iamkun#2010](iamkun#2010)) ([dd192a7](iamkun@dd192a7))
|
@BePo65 any chance you could update this PR soon? Or would you be fine if I tried to update it in a new PR? |
Added tests for more scenarios (e.g. issue iamkun#1596)
'd.apply' calls dayjs, which expects a config object as 2nd parameter; otherwise it creates a new config object without utc property. Using 'd.utc.apply' would call dayjs.utc, but cannot be used, if not parsing as utc. By setting 'args[1]' cfg is modified to include a single format; this config already has a correctly set utc property.
CustomParseFormat.parse expects a string or an array as args parameter of the cfg. When called recursively (via 'd.apply') dayjs added 'arguments' which is of type object as cfg.args and therefore parse cannot access e.g. the format as 'args[1]'. The rest operator changes arguments to an array. And if dayjs gets a config object with an args property as the 2nd parameter, we do not have to add an args property, as everything is already in place.
When the 2nd paramater of dayjs() is a config object, just assigning 'arguments' to the args property will modify the original 'c' parameter and thus create a self referencing property (does not change functionality, but should be avoided). As 'arguments' is of type 'object', we must handle the case 'dayjs() with multiple formats array, but without utc' by taking care of 'c' of type array.
The original implementation created an invalid date, when 'regex.exec(part)' did not find a match (resulting in match to be null). In that case 'value = match[0]' will throw.
This test uses the 'advancedFormat' plugin
I added 2 tests for your issue #1734 and found out that I missed a comment from @iamkun - replied to this comment too. So now I hope we have everything in place so that the review of this pr can be completed. Let's hope the best 😄 |
|
Is any help needed with this PR? |
CHANGELOG.md
Outdated
| @@ -1,3 +1,11 @@ | |||
| ## [1.11.7](https://github.com/iamkun/dayjs/compare/v1.11.6...v1.11.7) (2022-12-06) | |||
There was a problem hiding this comment.
I think the changes to CHANGELOG.md should be reverted as they're not in scope for this PR
There was a problem hiding this comment.
This line comes from dayjs 1.11.7 - the base of my pr and is autogenerated by the dayjs build system (I suppose). The line itself is the link to the git changes (as in any versions before). But you knew all this before 😄
So I don't know why this is a "changed file" here, as I didn't change it in my pr.
in the cuurent master branch (1.11.9) the line looks unchanged and AFAIK github should / could rebase a pr automatically on merge.
So what should I change in the changelog.md file?
There was a problem hiding this comment.
GitHub says these lines are added by your PR. Maybe if you rebase this PR to dev these changes would be removed since they are then already implemented
There was a problem hiding this comment.
Sounds good, as this pr will merge to dev.
But.... somehow it looks like dev is not up to date, For me it looks like the dev branch somehow is stuck on v1.11.6, while master moved on to 1.11.9.
So I think / hope that all this mess will disappear, when this pr is merged into master (if this will ever happen :-)
There was a problem hiding this comment.
Did you check out the dev from your fork by any chance? It seems that dev is the default branch of this repo and things have been merged to it a few days ago
There was a problem hiding this comment.
oh man this is a mess with the branches in this repo:
master contains the line to v1.11.9 while dev is not up to date with master.
I think I will rebase the pr to dev as soon as @iamkun answers to my comment on his remark to 'utils.js' below.
The issue #1596 shows that the plugin 'customParseFormat' does not give a valid date, when used with utc parsing dates with offset.
I identified 2 interdependant reasons for this:
customParseFormat.parse gives wrong arguments to dayjs when called by utc plugin with an array of formats.
Details:
calling
utc(...)for parsing the utc plugin creates a cfg object with a correct 'utc' property ('true'); but the recursion in the parse function of 'customParseFormat' just transfers all the arguments to the call to 'dayjs' (const result = d.apply(this, args)). But the args property must be a cfg object.parsing a date with offset in strict mode always returns 'Invalid Date'.
Details:
(isStrict && date != this.format(format))is an elegant way to find out, if the parsed date respects all the separators as defined in the format. But it fails, as soon as an offest comes into play; in this case the result of formatting the parsed date corresponds to the given format, but the used offset depends an the current local timezone and is not necessarily the same as the offset in the date to be parsed (e.g. '11:00+01' is the same as '12:00+02').As a result I had to change the way we identify a 'strict' match (including the handling of overflows in strict mode).
I did not squash the single commits, so that it is easier to analyze the single modifications.
The replacement for
(isStrict && date != this.format(format))(e.g. the part 'with offset' of the issue #1596) is in commit69fdc94and158aed8; the commits963b166and6a2974ejust handle the 'with utc' part of issue #1596) and52902e0the self reference.The rest are just tests for all the affected parsing scenarios.