Closed yuki0410-dev closed 1 month ago
@yuki0410-dev anything needed to complete this?
@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.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 98.36%. Comparing base (
cf820dd
) to head (6fe84be
). Report is 4 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
@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
.
@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 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)
@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.
@mirus-ua can you review this PR as well?
@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.)
prop: Type | null | undefined
can be simplified toprop?: 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.
Also, as for the
prop: Type | null | undefined
type,@types/react-datepickr
usedDate | 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
@yuki0410-dev @mirus-ua is this PR ready to merge?
@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 https://github.com/Hacker0x01/react-datepicker/pull/4757
name: Migrate calendar.jsx and index.jsx
Description
Linked issue: #4700
Changes
calendar.jsx
andindex.jsx
was migrated to tsContribution checklist