add types #4592

yuki0410-dev · 2024-03-14T14:53:33Z

Description

Linked issue: #4587

Problem

Changes

Based on @types/react-datepicker, incorporating changes from v6.3.0
- add types (d.ts)
- add test
- add CI
- fix md

Screenshots

To reviewers

Merging and releasing this PR will allow react-datepicker type completion to work without installing @types/react-datepicker.
You will also see the TS icon (painted blue) on npmjs.com.

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.

codecov · 2024-03-14T14:55:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.96%. Comparing base (5261cbd) to head (82582d0).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4592   +/-   ##
=======================================
  Coverage   96.96%   96.96%           
=======================================
  Files          28       28           
  Lines        2601     2601           
  Branches     1095     1095           
=======================================
  Hits         2522     2522           
  Misses         79       79

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

pullrequest

✅ This pull request was sent to the PullRequest network.

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

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 619
- 0

47% TSX (tests)
38% TypeScript
7% GitHubActions
3% JSON with Comments
5% Other

Generated lines of change

+ 8
- 0

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

pullrequest

Having read the associated issue for this PR, this sounds like a good idea to me. I'd definitely love to see more proper TypeScript support for this library (for those who wish to use it).

The implementation itself also seems fine to me. I had 2 minor comments below. But overall, the PR is introducing:

TypeScript tests to catch any type errors
A GitHub workflow to run these tests
The type declaration file itself

My only other suggestion would be to find any instances in the README or elsewhere (e.g., documentation) that involve notes on contributing, and mention that TypeScript types should be updated when making relevant changes. A good example of this is actually your recent PR (#4553).

The primary maintainers of this repository will need to have the final say on supporting TypeScript at this level. I think it seems like a beneficial addition to the project, but we will have to wait for their feedback before finalizing this.

Having said that, thanks for this contribution and for all of the excellent contributions you've been making lately to improve this library. It's very appreciated!

Ryan

Reviewed with ❤️ by PullRequest

pullrequest · 2024-03-14T21:58:05Z

types/types.tests.tsx

+    },
+    fn(state: MiddlewareState): MiddlewareReturn {
+        if (state.placement === "top") {
+            console.log("Popper is on the top");


ISSUE: no-console (Severity: Medium)
Unexpected 'console' statement.

Remediation:
Could this console.log statement be removed?

🤖 powered by PullRequest Automation 👋 verified by Ryan

pullrequest · 2024-03-14T21:58:05Z

types/types.tests.tsx

+    weekLabel=""
+    withPortal
+    portalId=""
+    portalHost={document.body.shadowRoot!}


ISSUE: @typescript-eslint/no-non-null-assertion (Severity: Low)
Forbidden non-null assertion.

Remediation:
Could we please avoid using type assertions? This should be checkable in advance.

🤖 powered by PullRequest Automation 👋 verified by Ryan

🔸 Best Practices (Important)

Ryan

yuki0410-dev · 2024-03-14T23:57:55Z

Thanks for the review.

My only other suggestion would be to find any instances in the README or elsewhere (e.g., documentation) that involve notes on contributing, and mention that TypeScript types should be updated when making relevant changes. A good example of this is actually your recent PR (#4553).

I agree and will edit the md file.

The primary maintainers of this repository will need to have the final say on supporting TypeScript at this level. I think it seems like a beneficial addition to the project, but we will have to wait for their feedback before finalizing this.

I agree with you.
I would be happy if the main maintainer of this repository would understand the advantages and disadvantages of this PR and adopt it.

pullrequest

PullRequest reviewed the updates made to #4592 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.

Reviewed by:

Ryan

pullrequest

This change looks good, just one comment for a line that looks suspicious.

Jacques

Reviewed with ❤️ by PullRequest

pullrequest · 2024-03-15T06:11:52Z

types/types.tests.tsx

+    ref={(instance) => {
+        if (instance !== null) {
+            // $ExpectType ReactDatePicker<true>
+            instance;


ISSUE: @typescript-eslint/no-unused-expressions (Severity: Medium)
Expected an assignment or function call and instead saw an expression.

Remediation:
Should this function return something?

🤖 powered by PullRequest Automation 👋 verified by Jacques

Understandable to raise this as a comment, but I actually deliberately didn't flag this one. This is normal when writing tests specifically for the type system.

The idea is to just produce a bunch of statements and see if they emit type errors.

If you look at the rest of the file, this will make more sense, since you'll see a bunch of disparate JSX as well.

Ryan

martijnrusschen · 2024-03-15T06:33:24Z

@yuki0410-dev I'm not an expert, but I understand the benefits of supporting this. Is there any reason why these definitions are also maintained in a separate repo like you linked? I guess that's the same of what we're now adding in this repo.

yuki0410-dev · 2024-03-15T12:17:15Z

@martijnrusschen
Thank you for your understanding regarding TS support.

When this PR is released, users of later versions will no longer need to use @types/react-datepicker. However, the current version of @types/react-datepicker must be retained for users of versions prior to that release. (No further updates will be made.)

Initially there is no difference between the two repositories, but each time a parameter such as Props is added in this repository, it will be reflected in the types in this repository, and the difference between the two repositories will become larger and larger.

In this case, we are adding the type usePointerEvent as an example.
363cbb6

martijnrusschen · 2024-03-16T12:09:28Z

Thanks, will wait for the recommended updated.

pullrequest

PullRequest reviewed the updates made to #4592 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.

Reviewed by:

Ryan

yuki0410-dev · 2024-03-17T07:16:31Z

@martijnrusschen
Thank you for your understanding.
I will make the corrections pointed out by linter and add the documentation.

pullrequest

PullRequest reviewed the updates made to #4592 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.

Reviewed by:

Ryan

yuki0410-dev · 2024-03-20T02:06:05Z

I did CI tweaks, Lint error handling, and md file corrections.

yuki0410-dev · 2024-03-20T03:47:57Z

Sorry, but I'm going to have to rethink the idea once and I'll make it a draft.

pullrequest

PullRequest reviewed the updates made to #4592 up until the latest commit (82582d0). No further issues were found.

Reviewed by:

Ryan

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-15T17:21:36Z

This is a bold idea, but maybe we should consider adopting TS instead of just merging the repo with .d.ts?

This is almost impossible to do at once, but with a proper community road map, we can do it for all files individually. Moreover, the most important part is the datepicker itself, and docs and even tests can stay written in js for any amount of time

martijnrusschen · 2024-04-15T17:33:26Z

Yes agree, I'm open to it but don't have the time to get this going. Open to suggestions.

mirus-ua · 2024-04-15T18:30:02Z

What path I see:

create an issue similar to the mui team [material-ui] Convert Material UI components to support Pigment CSS mui/material-ui#41273
prepare rollup and the rest tools to work with typescript files
from my experience, we should adapt ts from small modules to the bigger ones

mirus-ua · 2024-04-16T06:43:30Z

I can prepare a draft of the migration issue

martijnrusschen · 2024-04-16T10:52:30Z

@mirus-ua sounds good!

mirus-ua · 2024-04-17T09:29:07Z

@martijnrusschen here is the draft, let's discuss it inside the issue #4700

yuki0410-dev · 2024-04-23T00:22:42Z

#4700 is supported so that the type definitions will be output, and this PR will be closed.

pullrequest bot reviewed Mar 14, 2024

View reviewed changes

yuki0410-dev force-pushed the feat/add-types branch 2 times, most recently from cdc74f9 to 366e188 Compare March 14, 2024 14:59

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

pullrequest bot reviewed Mar 14, 2024

View reviewed changes

yuki0410-dev force-pushed the feat/add-types branch 2 times, most recently from 94e3079 to 363cbb6 Compare March 14, 2024 16:01

pullrequest bot reviewed Mar 14, 2024

View reviewed changes

pullrequest bot reviewed Mar 15, 2024

View reviewed changes

pullrequest bot reviewed Mar 16, 2024

View reviewed changes

yuki0410-dev marked this pull request as draft March 17, 2024 14:53

pullrequest bot reviewed Mar 17, 2024

View reviewed changes

yuki0410-dev force-pushed the feat/add-types branch from 363cbb6 to 3463c13 Compare March 19, 2024 16:16

yuki0410-dev added 2 commits March 20, 2024 01:45

add: types (base @types/react-datepicker@v6.2.0)

72a7bf2

add npmignore

fa58773

yuki0410-dev force-pushed the feat/add-types branch from 3463c13 to e7ef315 Compare March 19, 2024 16:45

yuki0410-dev added 4 commits March 20, 2024 01:49

add types CI

1c0c0b0

fix eslint error

8984e0a

fix types (for v6.3.0)

10d3cbd

fix md

82582d0

yuki0410-dev force-pushed the feat/add-types branch from e7ef315 to 82582d0 Compare March 19, 2024 16:49

yuki0410-dev marked this pull request as ready for review March 19, 2024 16:51

yuki0410-dev marked this pull request as draft March 20, 2024 03:46

pullrequest bot reviewed Mar 20, 2024

View reviewed changes

pullrequest bot reviewed Apr 3, 2024

View reviewed changes

mirus-ua mentioned this pull request Apr 17, 2024

[milestone] Convert the library to typescript #4700

Closed

29 tasks

yuki0410-dev closed this Apr 23, 2024

yuki0410-dev deleted the feat/add-types branch May 14, 2024 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add types #4592

add types #4592

yuki0410-dev commented Mar 14, 2024 •

edited

Loading

pullrequest bot left a comment

codecov bot commented Mar 14, 2024 •

edited

Loading

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

pullrequest bot left a comment

pullrequest bot Mar 14, 2024

pullrequest bot Mar 14, 2024 •

edited

Loading

yuki0410-dev commented Mar 14, 2024

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot Mar 15, 2024

pullrequest bot Mar 15, 2024

martijnrusschen commented Mar 15, 2024

yuki0410-dev commented Mar 15, 2024

martijnrusschen commented Mar 16, 2024

pullrequest bot left a comment

yuki0410-dev commented Mar 17, 2024

pullrequest bot left a comment

yuki0410-dev commented Mar 20, 2024

yuki0410-dev commented Mar 20, 2024

pullrequest bot left a comment

pullrequest bot left a comment

mirus-ua commented Apr 15, 2024

martijnrusschen commented Apr 15, 2024

mirus-ua commented Apr 15, 2024

mirus-ua commented Apr 16, 2024

martijnrusschen commented Apr 16, 2024

mirus-ua commented Apr 17, 2024

yuki0410-dev commented Apr 23, 2024

add types #4592

add types #4592

Conversation

yuki0410-dev commented Mar 14, 2024 • edited Loading

Description

Screenshots

To reviewers

Contribution checklist

pullrequest bot left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 14, 2024 • edited Loading

Codecov Report

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 Mar 14, 2024

Choose a reason for hiding this comment

🤖 powered by PullRequest Automation 👋 verified by Ryan

pullrequest bot Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

🤖 powered by PullRequest Automation 👋 verified by Ryan

🔸 Best Practices (Important)

yuki0410-dev commented Mar 14, 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 bot Mar 15, 2024

Choose a reason for hiding this comment

🤖 powered by PullRequest Automation 👋 verified by Jacques

pullrequest bot Mar 15, 2024

Choose a reason for hiding this comment

martijnrusschen commented Mar 15, 2024

yuki0410-dev commented Mar 15, 2024

martijnrusschen commented Mar 16, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

yuki0410-dev commented Mar 17, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

yuki0410-dev commented Mar 20, 2024

yuki0410-dev commented Mar 20, 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

mirus-ua commented Apr 15, 2024

martijnrusschen commented Apr 15, 2024

mirus-ua commented Apr 15, 2024

mirus-ua commented Apr 16, 2024

martijnrusschen commented Apr 16, 2024

mirus-ua commented Apr 17, 2024

yuki0410-dev commented Apr 23, 2024

yuki0410-dev commented Mar 14, 2024 •

edited

Loading

codecov bot commented Mar 14, 2024 •

edited

Loading

pullrequest bot left a comment •

edited

Loading

pullrequest bot Mar 14, 2024 •

edited

Loading