[typescript-migration] date_utils.js

[mirus-ua]

---

name: Pull Request about: Create a pull request to improve this repository

Description

Linked issue: https://github.com/Hacker0x01/react-datepicker/issues/4700

To reviewers

An important topic to discuss. Some of our codebase is using the date-fns violating their types

For example, toDate has the following implementation inside date-fns library

function toDate(argument) {
  const argStr = Object.prototype.toString.call(argument);

  // Clone the date
  if (
    argument instanceof Date ||
    (typeof argument === "object" && argStr === "[object Date]")
  ) {
    // Prevent the date to lose the milliseconds when passed to new Date() in IE10
    return new argument.constructor(+argument);
  } else if (
    typeof argument === "number" ||
    argStr === "[object Number]" ||
    typeof argument === "string" ||
    argStr === "[object String]"
  ) {
    // TODO: Can we get rid of as?
    return new Date(argument);
  } else {
    // TODO: Can we get rid of as?
    return new Date(NaN);
  }
}

and as you can see, else conditions are under question because here is the type counterpart

export declare function toDate<DateType extends Date>(
  argument: DateType | number | string,
): DateType;

The function shouldn't accept null or undefined, and our Date constructor newDate can return null, and then this null is used in several places violating the toDate's type contract.

So, should I suppress ts errors and leave everything as is it will save the status quo, but in the future may cause problems related to date-fns if they remove undocumented pieces of code

OR

I'll add some default values for such cases, and we won't violate ts contracts, but it may break someones business logic and we should release this under a major version

Contribution checklist

• [x] I have followed the contributing guidelines.
• [x] I have added sufficient test coverage for my changes.
• [x] I have formatted my code with Prettier and checked for linting issues with ESLint for code readability.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.22%. Comparing base (8081b73) to head (2bc2789).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #4707 +/- ## ========================================== - Coverage 97.34% 97.22% -0.13% ========================================== Files 23 22 -1 Lines 2603 2129 -474 Branches 1109 899 -210 ========================================== - Hits 2534 2070 -464 + Misses 69 59 -10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[martijnrusschen]

I'll add some default values for such cases, and we won't violate ts contracts, but it may break someones business logic and we should release this under a major version

I'm curious to see what happens with the existing tests if we change the default values. If there's indeed major changes we can roll out a breaking change, I think it's good to adhere to the logic as documented by date-fns and not depend on something that not officially supported.

[mirus-ua]

I'm curious to see what happens with the existing tests if we change the default values. If there's indeed major changes we can roll out a breaking change, I think it's good to adhere to the logic as documented by date-fns and not depend on something that not officially supported.

Here is the log of failed tests after all types of alignment that I did in this commit I'm not sure that my experience with the library is enough to safely say if these tests indicate any breaching changes

Summary of all failing tests
 FAIL  test/date_utils_test.test.js
  ● date_utils › newDate › should return null for invalid value passed

    expect(received).toBeNull()

    Received: 2024-04-19T18:21:21.514Z

      57 |   describe("newDate", () => {
      58 |     it("should return null for invalid value passed", () => {
    > 59 |       expect(newDate("21123asd")).toBeNull();
         |                                   ^
      60 |     });
      61 |   });
      62 |

      at Object.toBeNull (test/date_utils_test.test.js:59:35)

  ● date_utils › isMonthDisabled › should be enabled if after maxDate but same month

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      482 |     it("should be enabled if after maxDate but same month", () => {
      483 |       const day = newDate("2023-01-02");
    > 484 |       expect(isMonthDisabled(day, { maxDate: newDate("2023-01-01") })).toBe(
          |                                                                        ^
      485 |         false,
      486 |       );
      487 |     });

      at Object.toBe (test/date_utils_test.test.js:484:72)

  ● date_utils › isYearDisabled › should be enabled by default

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      561 |
      562 |     it("should be enabled by default", () => {
    > 563 |       expect(isYearDisabled(year)).toBe(false);
          |                                    ^
      564 |     });
      565 |
      566 |     it("should be enabled if on the min date", () => {

      at Object.toBe (test/date_utils_test.test.js:563:36)

  ● date_utils › isYearDisabled › should be enabled if on the max date

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      575 |
      576 |     it("should be enabled if on the max date", () => {
    > 577 |       expect(isYearDisabled(year, { maxDate: newYearsDay })).toBe(false);
          |                                                              ^
      578 |     });
      579 |
      580 |     it("should be disabled if after the max date", () => {

      at Object.toBe (test/date_utils_test.test.js:577:62)

  ● date_utils › isYearDisabled › should be enabled if in included dates

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      590 |
      591 |     it("should be enabled if in included dates", () => {
    > 592 |       expect(isYearDisabled(year, { includeDates: [newYearsDay] })).toBe(false);
          |                                                                     ^
      593 |     });
      594 |
      595 |     it("should be disabled if not in included dates", () => {

      at Object.toBe (test/date_utils_test.test.js:592:69)

  ● date_utils › isYearDisabled › should be enabled if date filter returns true

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      600 |     it("should be enabled if date filter returns true", () => {
      601 |       const filterDate = (d) => isSameYear(d, newYearsDay);
    > 602 |       expect(isYearDisabled(year, { filterDate })).toBe(false);
          |                                                    ^
      603 |     });
      604 |
      605 |     it("should be disabled if date filter returns false", () => {

      at Object.toBe (test/date_utils_test.test.js:602:52)

 FAIL  test/year_picker_test.test.js
  ● YearPicker › should not return disabled class if current date is after maxDate but same year

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      185 |         "react-datepicker__year-text--disabled",
      186 |       ),
    > 187 |     ).toBe(false);
          |       ^
      188 |   });
      189 |
      190 |   it("should return disabled class if specified excludeDate", () => {

      at Object.toBe (test/year_picker_test.test.js:187:7)

  ● YearPicker › should return disabled class if specified includeDate

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      250 |       expect(
      251 |         year.classList.contains("react-datepicker__year-text--disabled"),
    > 252 |       ).toBe(false);
          |         ^
      253 |     }
      254 |     for (let i = pos + 1; i < 12; i++) {
      255 |       const year = yearTexts[i];

      at Object.toBe (test/year_picker_test.test.js:252:9)

  ● YearPicker › keyboard-selected › should set the date to the selected year of the previous period when previous button clicked

    RangeError: Invalid time value

      219 |     localeObj = getLocaleObject(getDefaultLocale());
      220 |   }
    > 221 |   return format(date, formatStr, {
          |                ^
      222 |     locale: localeObj,
      223 |     useAdditionalWeekYearTokens: true,
      224 |     useAdditionalDayOfYearTokens: true,

      at format (node_modules/date-fns/format.js:361:11)
      at Object.formatDate (src/date_utils.ts:221:16)
      at Object.formatDate (test/year_picker_test.test.js:560:20)

  ● YearPicker › keyboard-selected › should set the date to the selected year of the next period when next button clicked

    RangeError: Invalid time value

      219 |     localeObj = getLocaleObject(getDefaultLocale());
      220 |   }
    > 221 |   return format(date, formatStr, {
          |                ^
      222 |     locale: localeObj,
      223 |     useAdditionalWeekYearTokens: true,
      224 |     useAdditionalDayOfYearTokens: true,

      at format (node_modules/date-fns/format.js:361:11)
      at Object.formatDate (src/date_utils.ts:221:16)
      at Object.formatDate (test/year_picker_test.test.js:594:20)

 FAIL  test/multiple_selected_dates.test.js
  ● Multiple Dates Selected › should handle text format for two selected dates

    expect(received).toBe(expected) // Object.is equality

    Expected: "01/01/2024, 01/15/2024"
    Received: "01/01/2024"

      40 |     });
      41 |
    > 42 |     expect(datePicker.getByRole("textbox").value).toBe(
         |                                                   ^
      43 |       "01/01/2024, 01/15/2024",
      44 |     );
      45 |   });

      at Object.toBe (test/multiple_selected_dates.test.js:42:51)

  ● Multiple Dates Selected › should handle text format for more than two selected dates

    expect(received).toBe(expected) // Object.is equality

    Expected: "01/01/2024 (+2)"
    Received: "01/01/2024"

      55 |     });
      56 |
    > 57 |     expect(datePicker.getByRole("textbox").value).toBe("01/01/2024 (+2)");
         |                                                   ^
      58 |   });
      59 | });
      60 |

      at Object.toBe (test/multiple_selected_dates.test.js:57:51)

 FAIL  test/month_test.test.js
  ● Month › should call the provided onDayMouseEnter (Mouse Event) function

    expect(jest.fn()).toHaveBeenLastCalledWith(...expected)

    Expected: 2023-12-31T22:00:00.000Z

    Number of calls: 0

      311 |     fireEvent.mouseEnter(month);
      312 |
    > 313 |     expect(onDayMouseEnterSpy).toHaveBeenLastCalledWith(
          |                                ^
      314 |       utils.getStartOfMonth(utils.setMonth(startDay, 0)),
      315 |     );
      316 |   });

      at Object.toHaveBeenLastCalledWith (test/month_test.test.js:313:32)

  ● Month › should call the provided onDayMouseEnter (Pointer Event) function

    expect(jest.fn()).toHaveBeenLastCalledWith(...expected)

    Expected: 2023-12-31T22:00:00.000Z

    Number of calls: 0

      333 |     fireEvent.pointerEnter(month);
      334 |
    > 335 |     expect(onDayMouseEnterSpy).toHaveBeenLastCalledWith(
          |                                ^
      336 |       utils.getStartOfMonth(utils.setMonth(startDay, 0)),
      337 |     );
      338 |   });

      at Object.toHaveBeenLastCalledWith (test/month_test.test.js:335:32)

  ● Month › should call the provided onMonthClick function

    expect(received).toBe(expected) // Object.is equality

    Expected: 6
    Received: NaN

      428 |     )[6];
      429 |     fireEvent.click(month);
    > 430 |     expect(utils.getMonth(monthClicked)).toBe(6);
          |                                          ^
      431 |   });
      432 |
      433 |   it("should return disabled month if current date is out of bound of minDate and maxDate", () => {

      at Object.toBe (test/month_test.test.js:430:42)

  ● Month › should not return disabled month if current date is after maxDate but same month

    expect(received).not.toBe(expected) // Object.is equality

    Expected: not true

      499 |     expect(
      500 |       month.classList.contains("react-datepicker__month-text--disabled"),
    > 501 |     ).not.toBe(true);
          |           ^
      502 |     expect(month.getAttribute("aria-disabled")).not.toBe("true");
      503 |     fireEvent.click(month);
      504 |     expect(onDayClickSpy).toHaveBeenCalled();

      at Object.toBe (test/month_test.test.js:501:11)

  ● Month › should return disabled month if specified includeDate

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      564 |       expect(
      565 |         month.classList.contains("react-datepicker__month-text--disabled"),
    > 566 |       ).toBe(false);
          |         ^
      567 |     }
      568 |     for (let i = 6; i < 12; i++) {
      569 |       const month = monthTexts[i];

      at Object.toBe (test/month_test.test.js:566:9)

  ● Month › Keyboard navigation › should select March when Enter is pressed

    TypeError: Cannot read properties of null (reading 'toString')

      1878 |
      1879 |       expect(preSelected).toBe(true);
    > 1880 |       expect(selectedDate.toString()).toBe(
           |                           ^
      1881 |         utils.newDate("2015-03-01").toString(),
      1882 |       );
      1883 |     });

      at Object.toString (test/month_test.test.js:1880:27)

  ● Month › Keyboard navigation › should select March when Space is pressed

    TypeError: Cannot read properties of null (reading 'toString')

      1912 |
      1913 |       expect(preSelected).toBe(true);
    > 1914 |       expect(selectedDate.toString()).toBe(
           |                           ^
      1915 |         utils.newDate("2015-03-01").toString(),
      1916 |       );
      1917 |     });

      at Object.toString (test/month_test.test.js:1914:27)

 FAIL  test/datepicker_test.test.js (5.85 s)
  ● DatePicker › when update the datepicker input text while props.minDate is set › should auto update calendar when the updated date text is after props.minDate

    expect(received).toBe(expected) // Object.is equality

    Expected: "January 1801"
    Received: "July 1993"

      1119 |       expect(
      1120 |         container.querySelector(".react-datepicker__current-month").innerHTML,
    > 1121 |       ).toBe("January 1801");
           |         ^
      1122 |     });
      1123 |
      1124 |     it("should not auto update calendar when the updated date text is before props.minDate", () => {

      at Object.toBe (test/datepicker_test.test.js:1121:9)

Test Suites: 5 failed, 22 passed, 27 total
Tests:       20 failed, 888 passed, 908 total
Snapshots:   0 total
Time:        6.358 s

[martijnrusschen]

How do you want to proceed here? I think it's fine to make the changes to make this fully compatible with date-fns, we could release a major upgrade once we're done with the TS conversion for example. Perhaps we hold on this on until the other ones are done and we release it as a major upgrade?

[mirus-ua]

How do you want to proceed here? I think it's fine to make the changes to make this fully compatible with date-fns, we could release a major upgrade once we're done with the TS conversion for example. Perhaps we hold on this on until the other ones are done and we release it as a major upgrade?

Yes, I thought from the beginning that this was the best idea. Hold the next release and make it a major one. We will still be 99,9% compatible and break only edge cases

[mirus-ua]

@martijnrusschen I was thinking about what to do with test cases because I see three groups of problems:

• a test is invalid because this is a breaking change, and we can't fix it
• a test is valid, but some props weren't passed, and that's why it fails
• a test is valid, and we can redo some inner logic to save the original behavior

Since we decided to go with a major release, the best way to deal with all three points is to skip tests until problematic components are rewritten in typescript. It'll save tons of time and effort instead of crawling through code manually

WDYT?

[martijnrusschen]

Would it make sense to hold on this PR until the other files are rewritten? That way, we can safely release the nonbreaking changes until we get to a point where we have to make these changes.

[mirus-ua]

Would it make sense to hold on this PR until the other files are rewritten? That way, we can safely release the nonbreaking changes until we get to a point where we have to make these changes.

We can try, but then we'll have some issues with types. Many files rely on these helpers, and we need proper types here so we don't play a guessing game.

[martijnrusschen]

True! Ok, let's continue with making the changes as they're designed to work according to date-fns and we can release a major upgrade after this PR.

[mirus-ua]

True! Ok, let's continue with making the changes as they're designed to work according to date-fns and we can release a major upgrade after this PR. I think it's no need anymore

The only potential breaking change is that we do not return null if the date format is invalid but return new Date instead

Just some spare time and focus

[martijnrusschen]

Makes sense! Good to document as part of a breaking change 👍

[mirus-ua]

Hooray. All green!

[martijnrusschen]

I've requested a last review from @pullrequestinc since this is a pretty big PR.