[typescript-migration] date_utils.js #4707
Conversation
![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.
@mirus-ua you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
PullRequest Breakdown
Reviewable lines of change
+ 723
- 204
96% TypeScript
3% JavaScript (tests)
1% TSX
<1% JSON
<1% JavaScriptType of change
Feature - These changes are adding a new feature or improvement to existing code.
| 1 Message | |
|---|---|
| Due to its size, this pull request will likely have a little longer turnaround time and will probably require multiple passes from our reviewers. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
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. |
There was a problem hiding this comment.
Re: the discussion question:
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
My preference would be to return a default, rather than a null. I think most JS / TS developers expect this when dealing with Dates.
Reviewed with ❤️ by PullRequest
src/date_utils.ts
Outdated
| interface LocaleObj extends Pick<Locale, "options" | "match" | "formatLong"> {} | ||
|
|
||
| function getLocaleScope() { | ||
| // Use this cast to avoid messing with users global types (like window) and the rest of keys in the globalThis object we don't care about |
src/date_utils.ts
Outdated
| /** | ||
| * Parses a date. | ||
| * | ||
| * @param value - The value. |
src/date_utils.ts
Outdated
| useAdditionalWeekYearTokens: true, | ||
| useAdditionalDayOfYearTokens: true, | ||
| }); | ||
| if (strictParsing) { |
There was a problem hiding this comment.
| time, | ||
| { excludeTimes, includeTimes, filterTime } = {}, | ||
| ) { | ||
| time: Date, |
There was a problem hiding this comment.
Giving Information: It looks like you're using Declaration merging for multiple interface definitions of DisabledOptions, both here and on line 1069.
This works as-is since TypeScript allows multiple interface definitions with the same name, but I wanted to flag other potential options:
- Have only the
DisabledOptionsdefinition defined on line 1069. - Have two
DisabledOptionsdefinitions, one with onlyminDateand onemaxDate - Change the name of the
DisabledOptionsinterface, similar toYearsDisabledAfterOptionsdefined on line 1177.
🔸 Giving Information (Important)
There was a problem hiding this comment.
almost missed the comment. fix here ac58035
| @@ -757,10 +1214,13 @@ | |||
| } | |||
There was a problem hiding this comment.
Should be safe to merge - mostly reformatting, commenting and small adjustments.
Reviewed with ❤️ by PullRequest
src/date_utils.ts
Outdated
| return new Date(); | ||
| } | ||
|
|
||
| const d = typeof value === "string" ? parseISO(value) : toDate(value); | ||
| return isValid(d) ? d : null; |
Here is the log of failed tests after all types of alignment that I did in this commit 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 |
|
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. |
|
@martijnrusschen I was thinking about what to do with test cases because I see three groups of problems:
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? |
|
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. |
|
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
changed the title
The only potential breaking change is that we do not return
Just some spare time and focus |
|
Makes sense! Good to document as part of a breaking change 👍 |
|
Hooray. All green! |
| "^.+\\.ts?$": "ts-jest", | ||
| // TODO: use it after the migration | ||
| // "^.+\\.ts?$": "ts-jest", |
There was a problem hiding this comment.
Is this something we'll want to address after everything is completed?
There was a problem hiding this comment.
Yes. It'll check types inside test cases to be even more safe than now.
Because right now, for example, we have a few tests with incorrect prop-types but tests are still green
There was a problem hiding this comment.
I even added it here #4700
|
I've requested a last review from @PullRequestInc since this is a pretty big PR. |
There was a problem hiding this comment.
Thank you for making the changes, and thank you for sharing the detailed feedback on whether or not to introduce breaking changes relative to documentation to date-fns
I agree in principle with the approach of meaking the changes, as designed to work according to date-fns. Whether or not others business logic is affected, it's valid to adhere to the logic as documented and not allow that to impede the progress of the library.
In my opinion, if a minor version change affects undocumented behavior, and a user of the library depended on that undocumented behavior, they can roll back to the prior minor version change.
Reviewed with ❤️ by PullRequest
name: Pull Request
about: Create a pull request to improve this repository
Description
Linked issue: #4700
To reviewers
An important topic to discuss.
Some of our codebase is using the date-fns violating their types
For example,
toDatehas the following implementation inside date-fns libraryand as you can see,
elseconditions are under question because here is the type counterpartThe function shouldn't accept
nullorundefined, and our Date constructornewDatecan returnnull, and then this null is used in several places violating thetoDate'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