fix: Inconsistent/broken behavior in `parseDate` #3988

maranomynet · 2023-04-06T14:04:25Z

Note
This is a re-opening of #3903 (since reverted by #3976) in the hope of it eventually getting accepted as a "breaking change".

See the expanded commit messages for details.

After this fix, the parsing behavior of react-datepicker…

a) is more consistent between locales/browsers/OS platforms
b) has fewer bugs
c) is the same whether dateFormat is string or an array of strings
d) treats arrayed dateFormat values as "most preferred first" (instead of the opposite).
e) is slightly more "strict".

Item "e" results from parseDate no longer falling back (with string values for props.dateFormat) on free-form, hard-to-predict new Date(value) parsing of input values when date-fns has already declared the value as "invalid".

It seems sensible (and more deterministic) to place the responsibility of parsing fully on date-fns and not (sometimes) second-guesss its results.

The new Date(value) parsing causes surprising and deceptively incorrect "false positive" parsing in situations where the dateFormat was dd.MM.yyyy and users type in a date in this format dd.MM.yy. Then their input gets "helpfully" re-parsed as MM.dd.yy, which was most definitely not intended by the developer setting up the datepicker input.

However, there's still a lot of flexibility in the parsing, as date-fns/parse provides very sensible flexibility based on localized RegExp configs, and also since props.dateFormat can be an Array of valid formattings.

As people are often relying on the old mis-behavior this PR needs to become part of a v5 release. A migration guide (and, possibly, updated documentation) could explain how to get the old behavior back by using props.dateFormat with multiple formats in order of decreasing priority.

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

codecov · 2023-04-06T14:06:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.09%. Comparing base (716c0cb) to head (422bbbc).
Report is 914 commits behind head on main.

❗ Current head 422bbbc differs from pull request most recent head 59658cd. Consider uploading reports for the commit 59658cd to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3988      +/-   ##
==========================================
- Coverage   94.21%   94.09%   -0.13%     
==========================================
  Files          20       20              
  Lines        1746     1726      -20     
  Branches      419      419              
==========================================
- Hits         1645     1624      -21     
- Misses         30       31       +1     
  Partials       71       71

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

martijnrusschen · 2024-03-10T19:39:42Z

It's been a long time since this was first submitted, but I agree with the thinking here. If you're still willing to bring this forward we can work towards releasing this as part of a major version bump.

pullrequest

✅ This pull request was sent to the PullRequest network.

@maranomynet you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 83
- 70

51% JavaScript
49% JavaScript (tests)

Type of change

Fix - These changes are likely to be fixing a bug or issue.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 83
- 70

51% JavaScript
49% JavaScript (tests)

Type of change

Fix - These changes are likely to be fixing a bug or issue.

pullrequest

This looks legit to me, I see no issues here.

Simon

Reviewed with ❤️ by PullRequest

pullrequest

I don't see any issues with this. Thanks for adding tests. I did add one minor comment for consideration.

Dallas

Reviewed with ❤️ by PullRequest

pullrequest · 2024-03-12T22:26:36Z

src/date_utils.js

+    const format = formats[i];
+    const parsedDate = parse(value, format, refDate, { locale: localeObject });
+    if (
+      isValid(parsedDate /* , minDate */) &&


Do you need to keep /* , minDate */ here?

🔹 Dead Code (Nice to have)

Dallas

pullrequest

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.

mirus-ua · 2024-04-22T19:02:00Z

@maranomynet hello
Can you update your PR based on this one #4707
and then we can do a major release, including your breaking changes

mirus-ua · 2024-05-04T07:38:41Z

@martijnrusschen hello
this PR looks promising. Should we try to adapt it for v7 with the rest of the breaking changes?

martijnrusschen · 2024-06-11T12:30:48Z

@mirus-ua I think that would be good.

WeatherGod · 2024-07-08T17:53:59Z

src/date_utils.js

-      parsedDate = new Date(value);
+  for (let i = 0, len = formats.length; i < len; i++) {
+    const format = formats[i];
+    const parsedDate = parse(value, format, refDate, { locale: localeObject });


While I have not tried this myself (I'm not experienced enough to know how), I believe this would fix a bug of mine that I discovered last week. I'm using UTCDate objects with the datepicker (set as selected, minDate, and maxDate), but dates entered manually in the input box are not getting parsed as UTCDates, and each iteration of manual date input results in an additional timezone offset to the datetime. I think this is because the original call to parse() passed in a new Date() for the reference date to date-fns's parse() instead of the component's minDate or some other UTCDate object.

My temporary workaround is to use startOfDay(new UTCDate(newDate)).toISOString() in the onChange function.

laug · 2024-08-15T09:11:35Z

It seems this PR was skipped over for v7, is there any chance of getting it included in a future v8?
I would like to work on fixing #3596 #3906 #4002 and this patch would be a prerequisite for fixing the parsing of ranges.

laug · 2024-08-15T16:58:45Z

src/date_utils.js

@@ -146,21 +107,15 @@ export function formatDate(date, formatStr, locale) {
  if (locale === "en") {
    return format(date, formatStr, { awareOfUnicodeTokens: true });
  }
-  let localeObj = getLocaleObject(locale);
+  const localeObj =
+    getLocaleObject(locale) || getLocaleObject(getDefaultLocale()) || null;


Adding || getLocaleObject(getDefaultLocale()) here will prevent the warning below from being shown when the locale string is invalid, as localeObj will not be nullish even when the locale string is invalid.

laug · 2024-08-15T17:15:10Z

src/index.jsx

@@ -496,6 +496,7 @@ export default class DatePicker extends React.Component {
      this.props.dateFormat,
      this.props.locale,
      this.props.strictParsing,
+      this.props.selected,
      this.props.minDate


parseDate only takes 5 parameters but is being passed 6 parameters here. This PR renames the 5th from minDate to refDate, but minDate is probably still needed to validate the parsed date correctly.

Edit: based on the commit message, minDate does not need to be passed to parseDate, so it should be removed here.

martijnrusschen · 2024-08-28T17:47:20Z

closing in favor of #5036

maranomynet added 4 commits April 6, 2023 13:49

fix: Remove weird, redundant code, fix bug illustrated in #45ce1fa3

bb7177f

maranomynet marked this pull request as draft April 11, 2023 11:58

martijnrusschen added the api break label Mar 10, 2024

pullrequest bot reviewed Mar 10, 2024

View reviewed changes

pullrequest bot reviewed Mar 12, 2024

View reviewed changes

pullrequest bot reviewed Mar 27, 2024

View reviewed changes

WeatherGod reviewed Jul 8, 2024

View reviewed changes

laug reviewed Aug 15, 2024

View reviewed changes

laug mentioned this pull request Aug 16, 2024

fix: Inconsistent/broken behavior in parseDate #5036

Open

3 tasks

martijnrusschen closed this Aug 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Inconsistent/broken behavior in `parseDate` #3988

fix: Inconsistent/broken behavior in `parseDate` #3988

maranomynet commented Apr 6, 2023 •

edited

Loading

codecov bot commented Apr 6, 2023 •

edited

Loading

martijnrusschen commented Mar 10, 2024

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot Mar 12, 2024

pullrequest bot left a comment

mirus-ua commented Apr 22, 2024

mirus-ua commented May 4, 2024

martijnrusschen commented Jun 11, 2024

WeatherGod Jul 8, 2024

laug commented Aug 15, 2024

laug Aug 15, 2024

laug Aug 15, 2024 •

edited

Loading

martijnrusschen commented Aug 28, 2024

fix: Inconsistent/broken behavior in parseDate #3988

fix: Inconsistent/broken behavior in parseDate #3988

Conversation

maranomynet commented Apr 6, 2023 • edited Loading

codecov bot commented Apr 6, 2023 • edited Loading

Codecov Report

martijnrusschen commented Mar 10, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

PullRequest Breakdown

pullrequest bot left a comment

Choose a reason for hiding this comment

PullRequest Breakdown

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Mar 12, 2024

Choose a reason for hiding this comment

🔹 Dead Code (Nice to have)

pullrequest bot left a comment

Choose a reason for hiding this comment

mirus-ua commented Apr 22, 2024

mirus-ua commented May 4, 2024

martijnrusschen commented Jun 11, 2024

WeatherGod Jul 8, 2024

Choose a reason for hiding this comment

laug commented Aug 15, 2024

laug Aug 15, 2024

Choose a reason for hiding this comment

laug Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

martijnrusschen commented Aug 28, 2024

fix: Inconsistent/broken behavior in `parseDate` #3988

fix: Inconsistent/broken behavior in `parseDate` #3988

maranomynet commented Apr 6, 2023 •

edited

Loading

codecov bot commented Apr 6, 2023 •

edited

Loading

laug Aug 15, 2024 •

edited

Loading