Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[milestone] Convert the library to typescript #4700

Open
27 of 29 tasks
mirus-ua opened this issue Apr 17, 2024 · 62 comments
Open
27 of 29 tasks

[milestone] Convert the library to typescript #4700

mirus-ua opened this issue Apr 17, 2024 · 62 comments

Comments

@mirus-ua
Copy link
Contributor

mirus-ua commented Apr 17, 2024

Contribution is welcome

Why?

Everything started here from the willing to merge a separate package with the corresponding type definitions into this repo.
The idea is great, but we, as a community, can go further and migrate the entire code base into typescript.

Any benefits?

  • only one package to install for end users
  • easier to maintain
  • your help to the lovely library
  • ???
  • profit

Our steps to achieve the goal

Modules to migrate and how to contribute

Basic rules and advice:

  • start with the smallest isolated modules, like calendar_container.jsx
  • prevent the use any or other loose types if you don't have strong arguments
  • prevent using "slow types", give this option to the end users
  • use interfaces instead of types because the large amount of the code base is class-based
  • add JSDocs where you can to improve overall DX. Everyone likes a good DX

Steps to contribute

  1. Write in the comments the name of the module that you want help to migrate
  2. Me or a maintainer will assign your name to the list to prevent doing the same job twice
  3. Migrate according to the list of advice above
  4. Highly recommended to migrate test cases together with the module
  5. Create a PR with a name format [typescript-migration] calendar.jsx etc.

Breaking changes so far:

  • newDate helper always returns a date, if the initial params were invalid, then it returns just a new Date Line; PR
  • calendarIconClassname => calendarIconClassName PR
@mirus-ua mirus-ua changed the title (draft)[milestone] Convert the library to typescript [milestone] Convert the library to typescript Apr 19, 2024
@mirus-ua
Copy link
Contributor Author

@martijnrusschen can you pin the issue?

@martijnrusschen
Copy link
Member

Nice progress so far!

@yuki0410-dev
Copy link
Contributor

@mirus-ua ( @martijnrusschen )
Thank you for the great attempt.
I would like to help as well, but should I declare the file to be started on this issue?

@yuki0410-dev
Copy link
Contributor

refs #4000

@martijnrusschen
Copy link
Member

@yuki0410-dev Feel free to claim a file and let it know here so there's no duplicate work done.

@mirus-ua
Copy link
Contributor Author

@yuki0410-dev hello
Yes, grab anything, the only one piece of advice is to pick modules without dependencies or with dependencies only on typescript modules, because it won't be pleasant to combine them later

@Olenka-Yurchuk
Copy link
Contributor

I would like to take the portal jsx file in work

@yuki0410-dev
Copy link
Contributor

I would like to take the week_number.jsx file in work

@mirus-ua
Copy link
Contributor Author

Added your name to week_number.jsx

@mirus-ua
Copy link
Contributor Author

6.6% 💪

Знімок екрана 2024-04-23 о 17 15 33

@yuki0410-dev
Copy link
Contributor

@mirus-ua
I would like to take the year_dropdown.jsx and year_dropdown_options.jsx file in work.

@yuki0410-dev
Copy link
Contributor

@mirus-ua
I would like to take the month_dropdown.jsx and month_dropdown_options.jsx file in work.

@mirus-ua
Copy link
Contributor Author

@martijnrusschen hello. Please ask reviews to look at four opened PRs if you can. It'll be much faster if we merge them because the rest of the modules depend on them

@martijnrusschen
Copy link
Member

Yep, I'll expect them to complete this soon. Normal turnaround time is ~2-4 hours.

@martijnrusschen
Copy link
Member

martijnrusschen commented Jun 2, 2024 via email

@mirus-ua
Copy link
Contributor Author

mirus-ua commented Jun 3, 2024

OK. I'll do then

@mirus-ua
Copy link
Contributor Author

mirus-ua commented Jun 5, 2024

It looks like I underestimated the amount of work. Almost all tests have types issues, so I don't think that I can do it relatively fast alone

@martijnrusschen
Copy link
Member

Is this something that could be split up somehow?

@mirus-ua
Copy link
Contributor Author

mirus-ua commented Jun 5, 2024

Is this something that could be split up somehow?

I can push the branch with basic config, and then we can use it as PRs target for this activity

@martijnrusschen
Copy link
Member

sounds good

@mirus-ua
Copy link
Contributor Author

mirus-ua commented Jun 6, 2024

I'll do it today

UPD:

#4870

@yuki0410-dev
Copy link
Contributor

@mirus-ua
Just a thought, but if we assign the tests to ts-jest one file at a time in the transform section, can we migrate from babel-jest to ts-jest without causing errors in CI?

@mirus-ua
Copy link
Contributor Author

mirus-ua commented Jun 7, 2024

@mirus-ua Just a thought, but if we assign the tests to ts-jest one file at a time in the transform section, can we migrate from babel-jest to ts-jest without causing errors in CI?

Do you mean here explicitly add files that we migrated? It could work even if it'll be tedious

@yuki0410-dev
Copy link
Contributor

@mirus-ua
That's right.
I am trying it now in #4871 and have a base. I am going to try one file now.

@yuki0410-dev
Copy link
Contributor

@mirus-ua @martijnrusschen
It looked like we could migrate babl-jest to ts-jest while merging into the Master branch, so we created PR #4871.

@martijnrusschen
Copy link
Member

Are we safe to cut a major release now we've converted the last tests?

@mirus-ua
Copy link
Contributor Author

Are we safe to cut a major release now we've converted the last tests?

I think so. We've done everything possible to integrate types safely into the existing code base and save the future feature development from type degradation

@martijnrusschen
Copy link
Member

Cool, for the release notes, we should capture the breaking changes.

@martijnrusschen
Copy link
Member

Here's the changelog: https://github.com/Hacker0x01/react-datepicker/releases/tag/v7.0.0

@mirus-ua
Copy link
Contributor Author

That was a good teamwork. Thanks all.

I think the issue should be open because we have some work with docs to do

@bsal649
Copy link

bsal649 commented Jun 14, 2024

I only had one custom component interfacing with ReactDatePicker but I encountered a lot of breaking changes that weren't listed in the release.

  1. ReactDatePicker name changed to DatePicker.
  2. ReactDatePickerProps name changed to DatePickerProps.

I had an interface as below:

interface DateInputProps extends Partial<ReactDatePickerProps> {
  style?: React.CSSProperties;
}

With an onChange with below def:

function handleChange(date: Date | null)
  1. I had to change the interface to a custom type.
  2. I had to make onChange and onSelect optional in the custom type.
  3. I had to explicitly set selectsRange and selectsMultiple to undefined in order to get default behaviour.
  4. I had to pass the excludesScrollbar prop in all the usages of my custom component.
  5. I assume that @types/react-datepicker needs to be uninstalled but not sure about this.

In the end my interface looks like this:

type CustomDatePickerProps = Omit<
  DatePickerProps,
  'onChange' | 'onSelect' | 'selectsRange' | 'selectsMultiple'
> & {
  onChange?: (
    date: Date | null,
    event?: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>,
  ) => void;
  onSelect?: (
    date: Date | null,
    event?: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>,
  ) => void;
  excludeScrollbar?: boolean;
  selectsRange?: boolean;
  selectsMultiple?: boolean;
};

type DateInputProps = CustomDatePickerProps & {
  style?: React.CSSProperties;
};

And my handleChange like this:

function handleChange(
    date: Date | null,
    event?:
      | React.MouseEvent<HTMLElement>
      | React.KeyboardEvent<HTMLElement>
      | undefined,
  )

@yuki0410-dev
Copy link
Contributor

yuki0410-dev commented Jun 14, 2024

@bsal649 ( @mirus-ua @martijnrusschen )
Thank you for the quick response.

I will answer as far as I am aware.

  1. ReactDatePicker name changed to DatePicker.

I think you forgot to put this in the "breaking changes" section.

  1. ReactDatePickerProps name changed to DatePickerProps.
  1. The interface needed to be changed to a custom type.

I think this is not "breaking changes" because this is a change from @types/react-datepicker, which is originally managed by someone outside this Repository member.

  1. I had to pass the excludesScrollbar prop in all the usages of my custom component.

I could not find that Props in this current Repository.

  1. I assume that @types/react-datepicker needs to be uninstalled but not sure about this.

It must be uninstalled.

@bsal649
Copy link

bsal649 commented Jun 14, 2024

@yuki0410-dev Thanks for the explanation, and fair enough.

Maybe it'd be worth putting a note in the release that people who were using @types/react-datepicker might have a lot more breaking changes.

@martijnrusschen
Copy link
Member

@yuki0410-dev For "ReactDatePicker name changed to DatePicker.", that doesn't seem like an intended change and will be causing everyone to make changes. Can we reduce the breaking changes by reverting to the original name? I don't see a need to force change that here. Thoughts? Same counts for "ReactDatePickerProps name changed to DatePickerProps."

@mirus-ua
Copy link
Contributor Author

@yuki0410-dev For "ReactDatePicker name changed to DatePicker.", that doesn't seem like an intended change and will be causing everyone to make changes. Can we reduce the breaking changes by reverting to the original name? I don't see a need to force change that here. Thoughts? Same counts for "ReactDatePickerProps name changed to DatePickerProps."

The right way, IMO, is to add these in the list of the breaking changes rather then violating semver rules

@martijnrusschen
Copy link
Member

That would be the alternative, but it forces everyone to update their code for just a name change.

@yuki0410-dev
Copy link
Contributor

yuki0410-dev commented Jun 14, 2024

@martijnrusschen @mirus-ua
My question is, since DatePicker is export defaulted, can't we name it freely on the import side?
I don't think any modifications have occurred due to the renaming of the DatePicker class, at least in doc-site.

Currently, class names have not been changed since v6.9.0.

export default class DatePicker extends React.Component {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants