fix: Inconsistent/broken behavior in parseDate
#5036
Conversation
Parsing `valueEn` as a Portugese date should fail, but accidentally gets parsed using the default/en-US locale, because parseDate internally retries and accidentally omits[^1] the original locale option that time around. [^1]: https://github.com/Hacker0x01/react-datepicker/blob/5c1d6d923931535f105f3dddbb6f3e10fd8dd25c/src/date_utils.js#L121
…making sure input values are applied in a more consistent manner, and with formatting that matches the currently active dateFormat (usually the default format). This only affects clarity/readability. All tests still pass.
Fix involves vastly simplifying the internal code-paths of `parseDate`, to prevent further and repeated divergence of behavior when parsing `dateFormat` as `Array<string>` vs. as `string` NOTE: Removing the (redundant) `minDate` parameter has no effect on the tests, as minDate/maxDate boundry checks are enforced elsewhere in the component's value-updating lifecycle. NOTE 2: Adding instead `refDate` (using `props.selected`) to fully utilize the features of `date-fns/parse`. NOTE 3: The old behavior of re-parsing borked values using `new Date()` was somewhat dubious as it gave different results depending on the Browser/OS running the code. See more here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/Date#parameters
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!
What to expect from this code review:
- Comments posted to any areas of potential concern or improvement.
- Detailed feedback or actions needed to resolve issues that are found.
- Turnaround times vary, but we aim to be swift.
@laug you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Looks good to me! My only suggestions were relatively minor.
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
The changes seem to clean up the parseDate behavior and makes this more consistent. Thank you for adding and adjusting relevant tests for the cleanup! I didn't see any blocking issues.
Reviewed with ❤️ by PullRequest
Initialize refDate with newDate() as a default argument. Suggested by @pullrequest @buu700.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5036 +/- ##
==========================================
+ Coverage 96.91% 96.97% +0.05%
==========================================
Files 29 29
Lines 3370 3336 -34
Branches 1396 1390 -6
==========================================
- Hits 3266 3235 -31
+ Misses 104 101 -3 ☔ View full report in Codecov by Sentry. |
|
Can you take a look at the merge conflicts? I think we can release this with a major upgrade soon. |
* main: (40 commits) chore(deps-dev): bump axe-core from 4.9.1 to 4.10.0 chore(deps-dev): bump @typescript-eslint/eslint-plugin chore(deps): bump micromatch from 4.0.7 to 4.0.8 chore(deps): bump webpack from 5.88.1 to 5.94.0 in /examples/hello-world chore(deps): bump webpack from 5.88.1 to 5.94.0 in /docs-site chore(deps): bump micromatch from 4.0.5 to 4.0.8 in /docs-site chore(deps): bump micromatch in /examples/hello-world chore(deps-dev): bump rollup from 4.19.0 to 4.21.1 🧪✅ Add test cases for safeQuerySelector, safeQuerySelectorAll, and SafeElementWrapper ♻️ Update a calendar_test test case to use SafeQuerySelector chain to avoid a temporary variable usage ✨ Add SafeElementWrapper utility to support safe query selection chain to avoid unnecessary temporary variables ♻️ Refactor the usage of safeQuerySelectorAll to also pass the required no of elements and remove the redundant throw error logic ♻️ Update the safeQuerySelectorAll helper to throw an error if the found element is less than the minExpected param ♻️ Refactor safeQuerySelector not toBe null test block to notThrow as the safeQuerySelector automatically throws an error if the element is not found ♻️ Remove the redundant check of the safeQuerySelector result to not be null fix: onSelect and onClickOutside are not optional Refactor: simplify calls to setOpen prop Refactor: simplify calls to remaining event handler props Refactor: simplify calls to onMonthChange handler Refactor: simplify calls to optional onSelect handler ...
* main: test: cover early return cases feat: support parsing of date range strings chore(deps-dev): bump stylelint from 16.7.0 to 16.9.0 test: parse date range chore(deps-dev): bump @types/node from 22.5.0 to 22.5.1 chore(deps-dev): bump lint-staged from 15.2.7 to 15.2.9 chore(deps-dev): bump eslint-plugin-unused-imports from 3.2.0 to 4.1.3
…Only`
Before this PR, the below function calls:
`index.tsx:handleChange -> date_utils:parseDate -> date-fns:parse`
were passing `new Date()` to date-fns parse function as the 'reference date' parameter.
This meant that when the datepicker had the `showTimeSelectOnly` prop and the date format was just a time (e.g. H:mm) the parse result's year/month/day would be today instead of the previously selected date, which we want.
To prevent this problem, the block of code being removed in the present commit would take the current selected date and reset the time to the newly input/parsed time, so that as a result, the year/month/day would not change.
Instead, this PR fixes the problem at its root, by passing `this.props.selected` instead of `new Date()` to the date-fns `parse` function as the reference date, such that the parse result of just a time string will have the same year/month/day as `this.props.selected`. This is the desired behavior, and so this block of code is no longer needed.
This is already tested in test "when update the datepicker input text while props.showTimeSelectOnly is set and dateFormat has only time related format".
* main: fix: migrate to husky 9 fix: remove postcss fix: remove unnecessary change fix: remove install-state.gz files fix: prettier error and some minor updates chore: update deps chore: update deps chore: upgrade deps chore: upgrade dependencies fix: test failures chore: upgrade babel fix: docs-site needs to store yarn installation fix: workflow should use yarn 4 not 1 chore: upgrade yarn to v4 and other dependencies
|
This should be ready to be merged, but let me know if any further work is required. |
* main: chore(deps-dev): bump husky from 9.1.3 to 9.1.5 chore(deps-dev): bump @babel/helpers from 7.25.0 to 7.25.6 chore(deps-dev): bump eslint-plugin-jest from 28.8.0 to 28.8.1 ♻️🧪 Refactor test with SafeElementWrapper and removed the direct usage of safeQuerySelector/safeQuerySelectorAll to avoid unnecessary intermediate variable declarations
* main: test: fix a test that would always fail if run on first day of month
src/index.tsx
Outdated
| @@ -604,15 +598,15 @@ export default class DatePicker extends Component< | |||
| .map((val) => val.trim()); | |||
| const startDateNew = parseDate( | |||
| valueStart ?? "", | |||
| dateFormat, | |||
| dateFormat!, | |||
There was a problem hiding this comment.
Rather than non-null assertions, consider checking earlier to see if each value is null and throwing an informative error in that case.
🔹 Best Practices (Nice to have)
There was a problem hiding this comment.
Hi Ryan,
This is now fixed.
FYI
Original code was written as below:
const { dateFormat = DatePicker.defaultProps.dateFormat, strictParsing = DatePicker.defaultProps.strictParsing, selectsRange, startDate, endDate, } = this.props;
This was failing Codecov as the parts of the lines after the equal signs was never reached in any test.
It seems it would be difficult to write a test that reaches them as the props are of type boolean and string (so we can't pass null) and have a default value if they are not specified, so will never be null or undefined in practice.
This is the point at which I removed the default values and added the non-null assertions.
The last commit uses a different syntax (non null coalescing) to assign the default values (instead of destructuring), and it seems codecov doesn't complain about this way of doing it.
There was a problem hiding this comment.
Got it, that makes sense, thanks for clarifying all that. It seems like a bit of a loophole, so might be worth seeing if the code coverage can still be expanded to hit that case in the future, but this does seem like a better solution than leaving a potential edge case bug in the production code.
There was a problem hiding this comment.
Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.
|
Hey all, any thoughts on when this might merge? This is quite valuable for data consistency |
|
@jseibert I think because this is a breaking change, it requires a major version bump. @martijnrusschen Is there any timeline for a v8.0 release? |
This is a rebasing of PR #3988 by @maranomynet on top of the current main branch.
Some minor changes relative to the commits in #3988:
asserttoexpectin tests.jsto.tsminDateparameter when callingparseDateOther than that, same changes and same commit messages with the same 4 commits as PR #3988.
Contribution checklist