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

Type definitions #37

Closed
Glazy opened this issue Sep 3, 2018 · 23 comments
Closed

Type definitions #37

Glazy opened this issue Sep 3, 2018 · 23 comments
Labels
Type: Enhancement General improvements or suggestions

Comments

@Glazy
Copy link
Contributor

Glazy commented Sep 3, 2018

It would be great to get some TypeScript type definitions written for each component.

@ryanflorence
Copy link
Member

I'm all for it, but we're going to need somebody dedicated to that, because I don't know typescript well enough to do it myself, or even review the work of others for it, and I'm not interested enough in typescript to learn it 😬(I know, I should be, and I would love it)

@Miklet
Copy link
Contributor

Miklet commented Jan 4, 2019

Just to have better look at current state of type definitions for all packages:

  • alert-dialog
  • alert
  • component-component
  • dialog
  • menu-button
  • portal
  • rect
  • skip-nav
  • utils
  • visually-hidden
  • window-size

@hedgerh
Copy link

hedgerh commented Jan 13, 2019

You start on component-component yet? I've never done a 3rd party typedef before but im about to use Component, so I figured I may as well start writing one out

@hedgerh
Copy link

hedgerh commented Jan 13, 2019

Could use some feedback on my @reach/component-component typedefs. I believe it's complete, but it may not be as elegant as it could be. #105

@hedgerh
Copy link

hedgerh commented Jan 13, 2019

Sorry to keep pinging this issue, but I went ahead and started all of the other remaining types. They are part of the same PR I linked above.

@SleeplessByte
Copy link

SleeplessByte commented Mar 28, 2019

declare module '@reach/tabs' {
  import { ComponentType } from "react"

  export interface BaseTabProps {
    children?: React.ReactNode
    as?: ComponentType
  }

  export interface TabsProps<A = React.HTMLAttributes<HTMLDivElement>> extends BaseTabProps & A {
    children: React.ReactNode

    defaultIndex?: number
    index?: number
    onChange?(index: number): void
  }
  export class Tabs extends React.Component<TabsProps> {}

  export type TabListProps<A = React.HTMLAttributes<HTMLDivElement>> = BaseTabProps & A
  export class TabList extends React.Component<TabListProps> {}

  export type TabPanelsProps<A = React.HTMLAttributes<HTMLDivElement>> = BaseTabProps & A
  export class TabPanels extends React.Component<TabPanelsProps> {}

  export type TabPanelProps<A = React.HTMLAttributes<HTMLDivElement>> = BaseTabProps & A
  export class TabPanel extends React.Component<TabPanelProps> {}

  export interface TabProps<A = React.HTMLAttributes<HTMLDivElement>> extends BaseTabProps & A {
    disabled?: boolean
  }
  export class Tab extends React.Component<TabProps> {}
}

Here are the types for @reach/tabs. Feel free to copy paste without attribution.

@hedgerh
Copy link

hedgerh commented Mar 30, 2019

added the tabs types to my PR

@hedgerh
Copy link

hedgerh commented Mar 30, 2019

So I've tested out all of the typedefs I did in my PR #105

What's the next move? Should we just merge them and see how they work or what? Need a reviewer

@johnridesabike
Copy link

johnridesabike commented Apr 20, 2019

Edit 2: The typedefs in #105 don't support props like style or className, which I think should be fixed. Are there any other "standard" props that should be added too?

Edit 1: I just copied @SleeplessByte's code into my project and it seems to have fixed my problem.

Original comment:

I'm not using TypeScript, but I use // @ts-check with VS Code to type-check my JavaScript. It keeps throwing warnings with Reach's tabs. Before I open a new issue addressing that, should this PR fix that bug?

Here's specific error that it gives me for Tabs, TabList, TabPanels, and TabPanel:

Type '{ children: Element[]; }' has no properties in common with type 'IntrinsicAttributes & RefAttributes<any>'.ts(2559)

TS can infer types from JS files, so adding typedefs isn't necessarily the only option to fix it if TS is just inferring them incorrectly.

@hedgerh
Copy link

hedgerh commented Apr 21, 2019

To make sure the response to @johnridesabike's issue is covered here, @SleeplessByte, your typings are off, actually. rest is not an explicit prop on the tabs. It's what's left after using the object rest property syntax on the props ...rest. The HTMLAttribute<HTMLDivElement> typing is actually not going anywhere, so you'll get syntax errors when trying to use props that should be available for a div element (className, style, etc.).

I updated my MR to fix this and wanted to give you the heads up. You can see the updates in here https://github.com/reach/reach-ui/pull/105/files#diff-fdb0763bfdc448564282bcf2ac0d3e53R1

@SleeplessByte
Copy link

My typings were generated from the documentation and not the code :)

@hedgerh thank you! I've not had this issue at all but yes you are correct :D

@swyxio
Copy link

swyxio commented Apr 21, 2019

@reach/tooltip:

[redacted, use SleeplessByte’s below]

@SleeplessByte
Copy link

@sw-yx this would have the same issue as my types above (before the edit).

declare module "@reach/tooltip" {

  export interface BaseTooltipProps {
    children: React.ReactNode
    label: React.ReactNode
    ariaLabel?: string
  }

  export interface TooltipProps<A = React.HTMLAttributes<HTMLDivElement>> extends BaseTooltipProps & A {
    children: React.ReactNode
  }
  export default class Tooltip extends React.Component<TooltipProps> {}
}

@john-osullivan
Copy link

You're all great, thanks for sharing your snippets <3

@hedgerh
Copy link

hedgerh commented May 16, 2019

Hey all, just an update: I think Ryan would rather wait until the library is more mature before worrying about types, so I've begun submitting typedefs for each component to DefinitelyTyped. Looks like the ones for @reach/alert have already been published under the package name @types/reach__alert. The double underscore part sucks, but you have to follow it when using the @namespace/package naming convention.

@swyxio
Copy link

swyxio commented May 16, 2019

cool. will you update us when the others have been published too? is this also the defacto umbrella issue for typings? lets make it easily discoverable where each typing is.

@hedgerh
Copy link

hedgerh commented May 16, 2019

This seems to be the only one that is a broad typings issue. I could start a new one to track my progress on creating the @types, if people would like. We should probably also add some docs on where to get them.

@SleeplessByte
Copy link

@types/reach__package is the standard. It could be added to the docs, but vscode and atom even suggest it when it's missing.

@setsun
Copy link

setsun commented Jun 4, 2019

Hey y'all 👋

When work picks up again on this, I wonder if it makes sense to have the type definitions bundled inside of reach-ui?

I've found that community provided types in DefinitelyTyped tend to lag behind the libraries they compliment, and that it might be easier to keep the types in sync if they are co-located.

The TypeScript docs have some decent guidance on packaging them with your library, by including a types property in the package.json: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

@setsun
Copy link

setsun commented Jun 4, 2019

Ah actually reading some of the previous comments and #105 I might be missing some context. I would still advocate for types packaged in the library, but in any case looking forward to seeing this work move forward!

@hedgerh
Copy link

hedgerh commented Jun 23, 2019

Whew, got all the typedefs together on DefinitelyTyped for the @reach packages. All of them but @reach/alert are in PR right now. @reach/alert got merged a week or two ago.

combobox - DefinitelyTyped/DefinitelyTyped#36379

tabs - DefinitelyTyped/DefinitelyTyped#36378

tooltip - DefinitelyTyped/DefinitelyTyped#36374

auto-id, rect, skip-nav, visually-hidden, window-size, utils - DefinitelyTyped/DefinitelyTyped#36373

menu-button - DefinitelyTyped/DefinitelyTyped#36372

component - DefinitelyTyped/DefinitelyTyped#36371

alert-dialog/dialog - DefinitelyTyped/DefinitelyTyped#36369

@types/reach__alert should already be available from a week or two ago.

You'll be able to get them by running yarn add @types/reach__combobox, etc.

@kambala3000
Copy link

component - DefinitelyTyped/DefinitelyTyped#36371

seems to be closed, any updates?

@chaance
Copy link
Member

chaance commented Oct 2, 2019

Hey all, thanks a ton for all the contributions and conversation! We are going to close this issue for now. I'm very likely to add types to all of our components over the next few weeks, but it's a pretty low priority at the moment. Types on DefinitelyTyped are in pretty good shape and fixes can be addressed there in the mean time.

Feel free to continue the conversation here -- I may well reopen it after I get more pressing issues fixed and the backlog cleared out. I want to have everything typed as much as anyone. 😬

@chaance chaance closed this as completed Oct 2, 2019
@bradwestfall bradwestfall added the Type: Enhancement General improvements or suggestions label Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement General improvements or suggestions
Projects
None yet
Development

No branches or pull requests