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

Is there Typescript support? #145

Closed
Luis-Palacios opened this issue Apr 24, 2019 · 8 comments
Closed

Is there Typescript support? #145

Luis-Palacios opened this issue Apr 24, 2019 · 8 comments
Labels
Milestone

Comments

@Luis-Palacios
Copy link

Luis-Palacios commented Apr 24, 2019

I noticed that a type definition exist and I have read through some conversations on issues and PR that suggest some attempts have been done in order to support typescript however I'm currently trying to use it and I'm running into multiple errors.

I would like to know if currently, this package type definition is correct because it does not seem to be

I run into this error if I try the basic example on
`"Type '{ value: string; label: string; children: { value: string; label: string; }[]; }[]' is not assignable to type 'Node[]'.
Type '{ value: string; label: string; children: { value: string; label: string; }[]; }' is not assignable to type 'Node'.
Types of property 'label' are incompatible.

image

If I change the way I import the package to const CheckboxTree = require('react-checkbox-tree'); then it works but without any type support

@Luis-Palacios Luis-Palacios changed the title Typescript support Is there Typescript support? Apr 24, 2019
@jakezatecky
Copy link
Owner

jakezatecky commented Apr 24, 2019

If you can suggest an appropriate fix to index.d.ts that will fix this issue, I would be glad to include it. As you likely discovered looking at the PR, I do not use TypeScript myself. If there was a way to automatically run tests against the definitions to ensure this does not happen, I would also greatly appreciate that as to prevent future regressions.

@Luis-Palacios
Copy link
Author

I'm looking into it, defining typed definition is actually pretty simple unless you use default exports and babel (see here #5565) as it is the case on this repository.

I'm going through the discussion and trying to understand what would be the best approach to fix it. If I success on understanding all of that magic I'll submit a PR

@djyako
Copy link

djyako commented May 20, 2019

I am curious about the update here? I am not getting the above error or exception here is my import import CheckboxTree, { Node } from "react-checkbox-tree";. However, I am not seeing the Id property on the component. For testing purposes how are you finding/grabbing the Nodes and/or their children?

@Luis-Palacios
Copy link
Author

I haven't had much time to do more proper research in order to create my formal PR however this is how I'm using it right now. I'm only using Icons and Language from the official Typings and I'm redefining the rest of interfaces

import { Icons, Language } from "react-checkbox-tree";
const CheckboxTree = require("react-checkbox-tree");

export { Icons, Language };

/** Redeclare type definitions from react-checkbox-tree because they are
 * currently wrong, if its fixed we could just use and re-export them
 * see: https://github.com/jakezatecky/react-checkbox-tree/issues/145
 * https://github.com/jakezatecky/react-checkbox-tree/issues/127
 * https://github.com/jakezatecky/react-checkbox-tree/pull/48
 */
export interface Node {
  label: React.ReactNode; // On the typed definitions this JSX.Element
  value: string | number;
  children?: Array<Node>;
  className?: string;
  disabled?: boolean;
  icon?: JSX.Element;
  showCheckbox?: boolean;
  title?: string;
}

export interface CheckboxProps {
  nodes: Array<Node>;
  checked: Array<string | number>;
  expanded: Array<string | number>;
  onCheck: (checked: Array<string>) => void;
  onExpand: (expanded: Array<string>) => void;

  disabled?: boolean;
  expandDisabled?: boolean;
  expandOnClick?: boolean;
  icons?: Icons;
  lang?: Language;
  name?: string;
  nameAsArray?: boolean;
  nativeCheckboxes?: boolean;
  noCascade?: boolean;
  onlyLeafCheckboxes?: boolean;
  optimisticToggle?: boolean;
  showExpandAll?: boolean;
  showNodeIcon?: boolean;
  showNodeTitles?: boolean;
  onClick?: (event: { checked: boolean; value: string | number }) => void;
}

jakezatecky added a commit that referenced this issue May 24, 2019
@jakezatecky
Copy link
Owner

@djyako the id property was indeed missing from the TypeScript interface. I have pushed a commit to add it, but this is not on any npm release yet.

As far as the issue @Luis-Palacios has, I am not quite sure what is different between React.ReactNode and JSX.Element as they would have seemed like they would be synonymous. JSX.Element was present in the initial PR to add TypeScript support so that is what I used for future enhancements.

If React.ReactNode works for strings, numbers, JSX elements, and other React components, then I can easily update the interface, as that is what PropTypes.node essentially means.

I am happy to provide TypeScript support given that the interface definitions look easy to maintain. Nevertheless, while I make an attempt to keep these typings in sync with the CheckboxTree PropTypes , I may sometimes forget, as was the case for id. Ideally, there would be something that would automatically test these typings or automatically generate them in order to prevent this type of disconnect in the future so @djyako's issue would not crop up again.

@XartaX
Copy link

XartaX commented Jun 28, 2019

I just started using this, and I changed the interface to React.ReactNode, and it seems to work. But I haven't really gotten to use it properly yet and the graphics look really weird (probably just because I have to connect the css), I'll look into it further next week. But yeah, setting it to ReactNode seems to let you set strings to label and it displays the string in the tree view.

@XartaX
Copy link

XartaX commented Jul 10, 2019

Followup:

This leads to issues when you have a build server since it will just npm install the packages which will still contain the un-updated interface. However, you can use the original interface if you explicitly send in an element to the label parameter.

I suggest updating the example code to explicitly use an element input as this won't cause all typescript users to crash.

I.e.

const nodes = [{`
    value: 'mars',
    label: 'Mars',
    children: [
        { value: 'phobos', label: <label>'Phobos'</label> },
        { value: 'deimos', label: <label>'Deimos'</label> },
    ],
}];

@jakezatecky
Copy link
Owner

Merged #169.

@jakezatecky jakezatecky added this to the v1.6.0 milestone Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants