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

Adding TypeScript Support (definitions) #11

Closed
PDDStudio opened this issue Jun 1, 2018 · 13 comments
Closed

Adding TypeScript Support (definitions) #11

PDDStudio opened this issue Jun 1, 2018 · 13 comments

Comments

@PDDStudio
Copy link

Hi,

this looks like an interesting project, however, I've had some problems getting things up and running with TypeScript.
Is there anything planned regarding support for TypeScript?

Cheers

@AWare
Copy link

AWare commented Jun 1, 2018

Quick note for anyone stubbing the typedef for now:

Typescript requests a .d.ts file containing declare module "reach__router" but actually wants declare module "@reach/router". (I may have configured typescript strangely so ymmv.)

@ryanflorence
Copy link
Member

I'd welcome some ts definitions in a PR.

@joshthecoder
Copy link

joshthecoder commented Jun 1, 2018

I wouldn't mind picking up this work unless someone else has already started on it.

Do we want to keep the type definition file in reach router itself or should it be contributed to the @types organization (DefinitelyTyped)?

@PDDStudio
Copy link
Author

@ryanflorence any reason why you closed this issue? I'd leave this open, so people know that there's actually something they can help with.
I tried declaring the typings myself - but wasn't able to get it working.

@AWare I also tried adding the declare module "reach__router" to my type definitions, but somehow VSCode still complained about missing typings. I did a quick search and stopped further investigation as because it seemed like typescript is having a general problem for resolving type definitions for scoped packages.
Did you get it to work? (If so, could you please provide some snippets/hints how)

@joshthecoder From my experience so far, I can tell that it's always more convenient to have typescript support by default, so there's no need to add an additional devDependency

@itsMapleLeaf
Copy link
Contributor

I've started writing up some type definitions on my end, and I'll probably end up making a PR. There are a few things I'm not 100% sure how to write types for (primarily the path property on route components), but we can discuss those in the PR.

@antmdvs
Copy link

antmdvs commented Jun 1, 2018

@kingdaro

primarily the path property on route components

i'm not really sure the best way to go about that either, but as i mentioned in #21, I was able to use module augmentation to suppress those errors (see https://t.co/MJGGv57cDV), but i feel like that's not something you'd want to do in a library

@joshthecoder
Copy link

@kingdaro I was thinking of just using Intersection Types to mixin the additional props expected of a Route component.

class Home extends React.Component<HomeProps & RouteProps> { }

RouteProps is provided by the reach router definitions and defines the props path, default, etc.

One issue that remains is how to also define the param props.

@itsMapleLeaf
Copy link
Contributor

@joshthecoder

I was thinking of just using Intersection Types to mixin the additional props expected of a Route component.

I thought of that as well, however that allows using the path property from within the component, which doesn't seem to be allowed from looking at the docs. If I'm mistaken, then that'll be the approach we'll go with.

One issue that remains is how to also define the param props.

That can be done through a generic on the props interface, and by using & to intersect them. See here

@joshthecoder
Copy link

@kingdaro true. The path param does seem to actually get passed to the component but this may change.

I think the only way we could hide those props from the component would probably be to augment the react module. Maybe instead of adding it to Attributes we add additional definitions for createElement that only applies to components that extend from our route component props. There we can intersect in the additional route params we want hidden (path, default).

I can try to code up an example of what I mean if this isn't clear.

Maybe @ryanflorence can speak to how important it would be to exclude path/default from the prop types.

@esamattis
Copy link

esamattis commented Sep 19, 2018

I tried to create simple render-prop wrapper to workaround the path prop issue

import {Router, Link, RouteComponentProps} from "@reach/router";

interface RouteProps {
    path: string;
    children: (route: RouteComponentProps) => React.ReactNode;
}

class Route extends React.Component<RouteProps> {
    render() {
        const {children, ...route} = this.props;
        return children(route);
    }
}

and usage like so

const Page1 = () => <div>Page 1</div>;
const Page2 = (props: {url: string}) => <div>Page 2 with url: {props.url}</div>;


const Main = () => (
                <Router>
                    <Route path="/page1">{route => <Page1 />}</Route>
                    <Route path="/page2">
                        {route => <Page2 url={route.uri || ""} />}
                    </Route>
                </Router>
);

This type checks but crashes in runtime with

Uncaught TypeError: children is not a function

It seems that Reach Router removes the children prop from the wrapping component...

@esamattis
Copy link

Uh, stupid me. Just realized why this cannot work: Reach Router allows you to nest routes with your own components and it does by checking the children. So of course this does not work because there's no way it can read the function closure. Not sure if there's any super clean way to type this.

@mrhut10
Copy link

mrhut10 commented Sep 20, 2018

any updates on this? i can't use the router currently because of the lack of types?

@andykenward
Copy link

There are typings for it

@types/reach__router

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

No branches or pull requests

9 participants