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

Typescript Support #71

Closed
waynevanson opened this issue Jun 8, 2019 · 12 comments
Closed

Typescript Support #71

waynevanson opened this issue Jun 8, 2019 · 12 comments
Assignees

Comments

@waynevanson
Copy link
Collaborator

Hi All,

I know there is an @types/sortablejs file out in the wild, but in the react world I don't know how to hook it up correctly, due to unfimilarity with the react specific implementation of SortableJs. I've used VueJs Component which works easily and am familiar with many SortableJS options.

From the example in this README.md, I entered this code:

import uniqueId from 'lodash/uniqueId';
import React from 'react';
import Sortable from 'react-sortablejs';

The squiggly lines of death are under 'react-sortablejs', which displays the following message on hover:

Try `npm install @types/react-sortablejs` if it exists or
add a new declaration (.d.ts) file containing `declare module 'react-sortablejs';

I am trying to go through the typescript documentation for an answer, but can't figure out how to extend the sortablejs types into react-sortablejs

Any ideas?

On resolution of this, we should link @types/sortablejs as a dependency in future builds alongside a declaration form for this package. I'll be happy to try doing this once I start using this Component.

@Godefroy
Copy link

Hi
I had the same issue, and I fixed it by adding the file src/react-sortablejs.d.ts

declare module 'react-sortablejs' {
  import Sortable from 'sortablejs';

  export interface SortableProps<ItemData, ListProps> {
    options?: Sortable.Options;
    onChange?: (list: ItemData[], sortable: Sortable, event: Sortable.SortableEvent) => void;
    tag?: string | React.ComponentType<ListProps>;
    style?: React.CSSProperties;
  }

  export default class SortableComponent<ItemData, ListProps> extends React.Component<
    SortableProps<ItemData, ListProps>
  > {}
}

I also installed @types/sortablejs and added this to tsconfig.ts:

    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,

I'm importing the Sortable component like this:

import Sortable from 'react-sortablejs';

Tell me if it works for you. We could submit this to @types/react-sortablejs.

@waynevanson
Copy link
Collaborator Author

Hi Godefroy,

That looks great! I will try it within 24 hours.

I'm a bit confused on how ItemData in interface SortableProps<ItemData, ListProps> would work, but I'm sure i'll answer my own question.

I actually converted the whole package into a Typescript file to the best of my ability. What are your thoughts on it? You can import it into your project using package name tsc-react-sortable

https://github.com/waynevanson/tsc-react-sortablejs

We could submit this to @types/react-sortablejs.

We should definitely submit this to @types/react-sortablejs. I'll try it and add recommendations to this issue.

I also feel we should clean up this component, as it's hard to read and manage. I feel like making a React.FunctionComponent would make the code easier to maintain and easier to add to.

@waynevanson
Copy link
Collaborator Author

From https://github.com/DefinitelyTyped/DefinitelyTyped#create-a-new-package

If you are the library author and your package is written in TypeScript, bundle the autogenerated declaration files in your package instead of publishing to Definitely Typed.

@types works, I'd prefer a rewrite in Typescript so when the source changes, so does the package. Otherwise we'll be writing 2 different packages for something small like this.

@Godefroy
Copy link

We have 3 solutions:

  1. Add typings to this repo
  2. Rewrite this repo to Typescript
  3. Create @types/react-sortablejs

You picked solution 2 but you still have a lot of any, and it's a different repo, so it's less maintanable in sync with this repo. My solution is probably more simple and has complete typings.

I guess we could remove the generic ItemData because it will always be string, as data-id is always a string (it's an HTML attribute, so it's a string).

@cheton What do you think?

@waynevanson
Copy link
Collaborator Author

Sorry, I closed by accident.

For a short term solution, I modified your declaration file given below. This is because it was giving me a JSX Constructor Error

declare module 'react-sortablejs' {
  import React from 'react';
  import Sortable from 'sortablejs';

  class ReactSortable extends React.Component<Props> {
    public options: Sortable.Options;
  }

  export interface Props extends React.ClassAttributes<Component> {
    onChange?: (list: ItemData[], sortable: Sortable, event: Sortable.SortableEvent) => void;
    options?: Sortable.Options;
    tag?: string | React.ComponentType<ListProps>;
    style?: React.CSSProperties;
    className?: string;
  }
  export = ReactSortable;
}

you still have a lot of any

There are some types that I do not understand yet. The goal is to have no any's.

and it's a different repo, so it's less maintanable in sync with this repo.

I made it as a proof of concept. Ideally with suggestion 2, this repo transforms into a Typescript project.

My solution is probably more simple and has complete typings.

With some modifications it works, but considering how small this project is (just a component), a rewrite wouldn't be too difficult and it's an all-in-one solution for the project. The benefits of a Typescript rewrite outweigh the cons.

I'd love to hear what @cheton has to say.

@waynevanson
Copy link
Collaborator Author

@owen-m1 it looks like @cheton is not maintaining this package, many issues are open etc. Even the build fails, according to the indicator.

I'd be happy to maintain this, or perhaps in the meantime you'd be able to help us push Typescript into this repository. What do you think about this?

Currently, I find the code on this package hard to work with. In comparison, the Vue.Draggable package is remarkable and works right out of box without configuration. In react-sortablejs, there are features such as sorting an input list to state and better nesting support which should be part of this package.

Have a look at https://github.com/waynevanson/tsc-react-sortablejs to see a basic conversion. It's the same as the current package and handles strict null checks. there are some any's as it was probably written without 100% type safety in mind.

Aside from my current efforts, I'd much prefer this being either a FunctionComponent or break down the Class component into testable parts. Which route do you think is a good idea?

@owen-m1
Copy link
Member

owen-m1 commented Jun 26, 2019

@waynevanson I don't know if he is still maintaining this or not. You'll have to email him about this, this is his project. If he approves, I can add you to the project.

I can help in any way as the "Sortable expert" but I don't know React :).

@waynevanson
Copy link
Collaborator Author

Thanks, @owen-m1 . I've sent him an email asking if I'd be able to switch maintainers, or if he doesn't want to switch, if he can reply to this thread.

@owen-m1
Copy link
Member

owen-m1 commented Jun 27, 2019

@waynevanson Would this package need Typescript support if Sortable uses Typescript? Or do you mean writing this package itself in Typescript?

@cheton
Copy link
Collaborator

cheton commented Jun 27, 2019

Hi @waynevanson

I'm currently busy at work in the last few months and the situation will not end until August, so I don't have much time to watch and respond GitHub issues. Also, I'm not an expert in TypeScript, I'd be happy if someone can help maintain / contribute to this project to complete TypeScript support, I can help do code review and publish new releases to NPM.

@owen-m1 Can you help add @waynevanson as a maintainer for this repo. Thank you.

@owen-m1
Copy link
Member

owen-m1 commented Jun 27, 2019

Sure, @waynevanson I sent you an invite.

@waynevanson
Copy link
Collaborator Author

There we are boys: #83

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

4 participants