[typescript-migration] calendar.jsx and index.jsx #4766

yuki0410-dev · 2024-05-05T14:13:32Z

name: Migrate calendar.jsx and index.jsx

Description

Linked issue: #4700

Changes calendar.jsx and index.jsx was migrated to ts

Contribution checklist

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

pullrequest

⚠️ This pull request was not sent to the PullRequest network because the pull request is a draft.

martijnrusschen · 2024-05-07T09:04:57Z

@yuki0410-dev anything needed to complete this?

yuki0410-dev · 2024-05-07T17:12:42Z

@martijnrusschen
I am working with index.jsx. There is some rework to be done for consistency, etc. I will release the draft after index.js is complete.

codecov · 2024-05-12T14:05:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.36%. Comparing base (cf820dd) to head (6fe84be).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4766      +/-   ##
==========================================
+ Coverage   96.35%   98.36%   +2.01%     
==========================================
  Files           8        6       -2     
  Lines         959       61     -898     
  Branches      435       19     -416     
==========================================
- Hits          924       60     -864     
+ Misses         35        1      -34

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

mirus-ua · 2024-05-13T08:43:29Z

I noticed a lot of changes that replaced randomProp?: type with randomProp?: type | null. IMO, it is better to replace null with undefined in place of usage instead of making all the types wider.

yuki0410-dev · 2024-05-13T14:14:00Z

@mirus-ua
I am glad for your advice.
I agree with unifying the internal types to undefined or null.
However, since the original selected and other props were of type Date | null | undefined, we have modified the related props to Date | null | undefined to maintain compatibility.
If you have any particular concerns, it would be appreciated if you could comment on them in the File changes tab.

yuki0410-dev · 2024-05-13T14:20:30Z

@mirus-ua
I am struggling with the part of index.ts where the onChange type changes due to selectsRange,selectsMultiple Props.
Any good advice would be appreciated.
In my current code, I am using conditional type with reference to @types/react-datepicker, but the type inference does not work well at the point where onChage is called.
Also, I defined some (conditional type) props using type = and tried to inherit them in the interface, but I get a ts(2312) error.
Otherwise, if I declare Props with type instead of interface, the inference works correctly, but it is not consistent because other Props are declared with interface.

mirus-ua · 2024-05-13T14:31:12Z

@mirus-ua I am struggling with the part of index.ts where the onChange type changes due to selectsRange,selectsMultiple Props. Any good advice would be appreciated. In my current code, I am using conditional type with reference to @types/react-datepicker, but the type inference does not work well at the point where onChage is called. Also, I defined some (conditional type) props using type = and tried to inherit them in the interface, but I get a ts(2312) error. Otherwise, if I declare Props with type instead of interface, the inference works correctly, but it is not consistent because other Props are declared with interface.

Conditional types work well only with type notation, so if the rest is OK and the only concern is consistency, so I suggest migrating to types instead of interfaces, I don't think that someone will be against it (btw I pick type over interface almost always)

yuki0410-dev · 2024-05-13T14:42:51Z

@mirus-ua
Thanks for the advice.
I too use type when writing code in my daily life.
However, when creating libraries, it is more convenient to be able to extend the type on the user side (as I have done in this PR by extending react-onclickoutside), so I was wondering if I could use interface.
For now, I will try to implement it with type, and if there are requests for interface, I will consider it again at a later time.

pullrequest

✅ 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.

@yuki0410-dev you can click here to see the review status or cancel the code review job.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 947
- 1,213

87% TSX
10% TypeScript
2% JavaScript (tests)
1% JSON
<1% Other

Type 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 be reviewed by PullRequest staff before being sent to the reviewer network, and the team will reach out if there are any concerns. After this pull request is sent to the network, please be aware that it will likely have a longer turnaround time and require multiple passes from our reviewers.

pullrequest

Very nice work tackling this typescript conversion! I left some feedback throughout for you to consider. Most of it is simplification suggestions but I also left some comments around potential regressions.

Joey G

Reviewed with ❤️ by PullRequest

src/calendar.tsx

src/date_utils.ts

pullrequest · 2024-05-14T16:37:07Z

src/date_utils.ts

-  { dateFormat, locale }: { dateFormat: string | string[]; locale?: Locale } = {
-    dateFormat: "yyyy-MM-dd",
-  },
+  date: Date | null | undefined,


I would consider creating a shared type like OptionalDate to remove the duplication in this file.

🔹 Code Reuse (Nice to have)

Joey G

IMHO, we should use prop?: Type over prop: Type | undefined

src/day.tsx

src/index.tsx

src/month_year_dropdown.tsx

src/month_year_dropdown_options.tsx

src/year.tsx

test/calendar_test.test.js

pullrequest

Left a few comments, but going back and looking at the commits one by one, it may be that these changes are more a question for the existing design than your changes.

The separation of commits, as you did, is a helpful tool for reviewers which could be improved a bit by increasing specifics. Eg, rather than "fix linting rules", I generally prefer, "Updated linting rules to include tsx files."

Hope this review helps you.

John G

Reviewed with ❤️ by PullRequest

src/calendar.tsx

pullrequest

PullRequest reviewed the updates made to #4766 up until the latest commit (429a0cb). No further issues were found.

Reviewed by:

John G

martijnrusschen · 2024-05-15T06:04:20Z

@mirus-ua can you review this PR as well?

mirus-ua

First of all, thank you for the big chunk of work and that you wrote a lot of explicit return types it'll help in the future to make changes in a safe way.

I have a few points:

prop: Type | null | undefined can be simplified to prop?: Type | null
I don't understand the reason why we replace HTMLDivElemnt with HTMLElement that quite generic and may cause problems for the extension in the future
VoidFunction as props looks like it should be replaced with proper typed function with arguments

Edit:
The good idea is to document all changes that even slightly can be called as breaking

package.json

src/date_utils.ts

mirus-ua · 2024-05-15T07:29:18Z

src/date_utils.ts

-  { dateFormat, locale }: { dateFormat: string | string[]; locale?: Locale } = {
-    dateFormat: "yyyy-MM-dd",
-  },
+  date: Date | null | undefined,


IMHO, we should use prop?: Type over prop: Type | undefined

src/date_utils.ts

src/calendar.tsx

src/index.tsx

src/month_year_dropdown.tsx

src/portal.tsx

src/week_number.tsx

tsconfig.json

yuki0410-dev · 2024-05-15T15:37:49Z

@mirus-ua
Thanks for the review.

I don't understand the reason why we replace HTMLDivElemnt with HTMLElement that quite generic and may cause problems for the extension in the future

Changed the HTMLElement type to a concrete type whenever possible.

VoidFunction as props looks like it should be replaced with proper typed function with arguments

We do not think it is necessary to add arguments at this time that are not being used at this time.
(I don't think this is the scope of the TS conversion.)

yuki0410-dev · 2024-05-15T15:47:19Z

prop: Type | null | undefined can be simplified to prop?: Type | null

I use args: Type | undefined and args?: Type explicitly.
The former always requires an argument at call time, whereas the latter does not raise an error if you forget to write the argument.
Since Optional is not a mere shorthand for union type with undefined, we believe that it should be used with the best of intentions.

Also, as for the prop: Type | null | undefined type, @types/react-datepickr used Date | null | undefined for selected props, etc., so we do so to maintain compatibility.
And the part where the value is passed is of a similar type.

pullrequest

PullRequest reviewed the updates made to #4766 up until the latest commit (b2b9b8d). No further issues were found.

Reviewed by:

Joey G

mirus-ua · 2024-05-15T16:34:55Z

Also, as for the prop: Type | null | undefined type, @types/react-datepickr used Date | null | undefined for selected props, etc., so we do so to maintain compatibility. And the part where the value is passed is of a similar type.

But the optional operator implicitly adds undefined to the union, or I get your point wrong

We do not think it is necessary to add arguments at this time that are not being used at this time.
(I don't think this is the scope of the TS conversion.)

If you say that all these functions don't receive any parameters then OK

pullrequest

PullRequest reviewed the updates made to #4766 up until the latest commit (6fe84be). No further issues were found.

Reviewed by:

John G

src/day.tsx

src/with_floating.tsx

src/week_number.tsx

src/calendar.tsx

src/index.tsx

martijnrusschen · 2024-05-16T07:24:21Z

@yuki0410-dev @mirus-ua is this PR ready to merge?

mirus-ua · 2024-05-16T07:26:59Z

@yuki0410-dev @mirus-ua is this PR ready to merge?

I don't see any major issues.
Some of the unsolved problems are addressed here #4757

pullrequest bot reviewed May 5, 2024

View reviewed changes

yuki0410-dev force-pushed the feat/4700_calendar branch from 434c39f to 3bc7937 Compare May 12, 2024 14:05

yuki0410-dev force-pushed the feat/4700_calendar branch from 3bc7937 to cc88150 Compare May 13, 2024 00:19

yuki0410-dev force-pushed the feat/4700_calendar branch 8 times, most recently from 843472f to 42d699c Compare May 14, 2024 15:21

yuki0410-dev marked this pull request as ready for review May 14, 2024 15:23

pullrequest bot reviewed May 14, 2024

View reviewed changes

src/year.tsx Show resolved Hide resolved

test/calendar_test.test.js Show resolved Hide resolved

pullrequest bot reviewed May 14, 2024

View reviewed changes

src/calendar.tsx Show resolved Hide resolved

src/calendar.tsx Show resolved Hide resolved

src/calendar.tsx Show resolved Hide resolved

yuki0410-dev force-pushed the feat/4700_calendar branch from 42d699c to 429a0cb Compare May 14, 2024 23:40

pullrequest bot reviewed May 15, 2024

View reviewed changes

mirus-ua approved these changes May 15, 2024

View reviewed changes

yuki0410-dev force-pushed the feat/4700_calendar branch from 429a0cb to 473c405 Compare May 15, 2024 14:26

yuki0410-dev force-pushed the feat/4700_calendar branch 2 times, most recently from c9a5a62 to b2b9b8d Compare May 15, 2024 15:33

pullrequest bot reviewed May 15, 2024

View reviewed changes

yuki0410-dev added 4 commits May 16, 2024 01:02

fix: types

2d9cf1b

fix tsconfig

279b165

fix eslint and error

1890d19

feat: convert calendar.jsx into ts

06b1f10

yuki0410-dev force-pushed the feat/4700_calendar branch from b2b9b8d to 6333420 Compare May 15, 2024 16:02

feat: convert index.jsx into ts

6fe84be

yuki0410-dev force-pushed the feat/4700_calendar branch from 6333420 to 6fe84be Compare May 15, 2024 16:05

pullrequest bot reviewed May 15, 2024

View reviewed changes

martijnrusschen reviewed May 15, 2024

View reviewed changes

src/day.tsx Show resolved Hide resolved

martijnrusschen reviewed May 15, 2024

View reviewed changes

src/with_floating.tsx Show resolved Hide resolved

src/week_number.tsx Show resolved Hide resolved

src/calendar.tsx Show resolved Hide resolved

src/index.tsx Show resolved Hide resolved

martijnrusschen merged commit 610d001 into Hacker0x01:main May 16, 2024
6 checks passed

mirus-ua mentioned this pull request May 16, 2024

[milestone] Convert the library to typescript #4700

Closed

29 tasks

yuki0410-dev deleted the feat/4700_calendar branch May 16, 2024 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript-migration] calendar.jsx and index.jsx #4766

[typescript-migration] calendar.jsx and index.jsx #4766

yuki0410-dev commented May 5, 2024

pullrequest bot left a comment

martijnrusschen commented May 7, 2024

yuki0410-dev commented May 7, 2024

codecov bot commented May 12, 2024 •

edited

Loading

mirus-ua commented May 13, 2024 •

edited

Loading

yuki0410-dev commented May 13, 2024

yuki0410-dev commented May 13, 2024 •

edited

Loading

mirus-ua commented May 13, 2024

yuki0410-dev commented May 13, 2024

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

pullrequest bot left a comment

pullrequest bot May 14, 2024

mirus-ua May 15, 2024

pullrequest bot left a comment

pullrequest bot left a comment

martijnrusschen commented May 15, 2024

mirus-ua left a comment •

edited

Loading

mirus-ua May 15, 2024

yuki0410-dev commented May 15, 2024

yuki0410-dev commented May 15, 2024

pullrequest bot left a comment

mirus-ua commented May 15, 2024

pullrequest bot left a comment

martijnrusschen commented May 16, 2024

mirus-ua commented May 16, 2024

[typescript-migration] calendar.jsx and index.jsx #4766

[typescript-migration] calendar.jsx and index.jsx #4766

Conversation

yuki0410-dev commented May 5, 2024

name: Migrate calendar.jsx and index.jsx

Description

Contribution checklist

pullrequest bot left a comment

Choose a reason for hiding this comment

martijnrusschen commented May 7, 2024

yuki0410-dev commented May 7, 2024

codecov bot commented May 12, 2024 • edited Loading

Codecov Report

mirus-ua commented May 13, 2024 • edited Loading

yuki0410-dev commented May 13, 2024

yuki0410-dev commented May 13, 2024 • edited Loading

mirus-ua commented May 13, 2024

yuki0410-dev commented May 13, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment • edited Loading

Choose a reason for hiding this comment

PullRequest Breakdown

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot May 14, 2024

Choose a reason for hiding this comment

🔹 Code Reuse (Nice to have)

mirus-ua May 15, 2024

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

martijnrusschen commented May 15, 2024

mirus-ua left a comment • edited Loading

Choose a reason for hiding this comment

mirus-ua May 15, 2024

Choose a reason for hiding this comment

yuki0410-dev commented May 15, 2024

yuki0410-dev commented May 15, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

mirus-ua commented May 15, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

martijnrusschen commented May 16, 2024

mirus-ua commented May 16, 2024

codecov bot commented May 12, 2024 •

edited

Loading

mirus-ua commented May 13, 2024 •

edited

Loading

yuki0410-dev commented May 13, 2024 •

edited

Loading

pullrequest bot left a comment •

edited

Loading

mirus-ua left a comment •

edited

Loading