Add typescript types

[itsMapleLeaf]

Fixes #11

As I mentioned in another issue, one thing I'm not sure about is how to handle the path property on route components. Here's a usage example of what I came up with:

import { RouteComponentMatchingProps, Router } from "@reach/router"
import React from "react"
import { render } from "react-dom"

let Home = () => <div>Home</div>
let Dash = () => <div>Dash</div>

const HomeRoute = Home as React.SFC<RouteComponentMatchingProps>
const DashRoute = Dash as React.SFC<RouteComponentMatchingProps>

render(
  <Router>
    <HomeRoute path="/" />
    <DashRoute path="dashboard" />
  </Router>,
  document.getElementById("app-root"),
)

Someone also suggested module augmentation as an alternative, though that would mean allowing the path property to appear on any component in the entire app, however it would certainly look cleaner in usage.

Comments on how to best handle this (and on other general corrections) are welcome.

[joshthecoder]

@kingdaro I kind of coded up a possible augmentation we could do to react without injecting the matching props into every component: https://gist.github.com/joshthecoder/88f2be7428e7c9b3fb8840ce16ff1d96

I explained this on the other issue but to summarize again we are defining new createElement types instead that match to only components that have props which extend from our route component props.

I haven't tested this at all yet so not 100% sure if this is viable yet.

[joshthecoder]

Should we also include some test files to verify the d.ts file is correct? We could follow the same structure used by https://github.com/DefinitelyTyped/DefinitelyTyped. Maybe place the files in a "types" folder.

We would need to add typescript as a devDependency so we can have the compiler on hand to type check the test files.

[itsMapleLeaf]

@joshthecoder

I kind of coded up a possible augmentation we could do to react without injecting the matching props into every component: https://gist.github.com/joshthecoder/88f2be7428e7c9b3fb8840ce16ff1d96

I explained this on the other issue but to summarize again we are defining new createElement types instead that match to only components that have props which extend from our route component props.

I haven't tested this at all yet so not 100% sure if this is viable yet.

I have a test bed project, and I'm not able to get this working correctly. I might be doing something incorrectly on my end, so try it out on your end instead to see if you can get it working.

Should we also include some test files to verify the d.ts file is correct? We could follow the same structure used by https://github.com/DefinitelyTyped/DefinitelyTyped. Maybe place the files in a "types" folder.

We would need to add typescript as a devDependency so we can have the compiler on hand to type check the test files.

Good idea 👍

@ryanflorence Comments on folder structure? Specifically, where index.d.ts should go, and where the typings tests should go

[joshthecoder]

@kingdaro yeah doesn't seem to be working for me complains that path isn't an allowed prop when I try to set it on a route component :|

I'm thinking maybe we just mix in these props with RouteComponentRouteProps (I'm actually thinking we just call this RouteComponentProps?). Those are actual props even if you shouldn't really need to use them. Not sure there is too much harm.

I'll fiddle around a bit more and see if I can get it working.

[ryanflorence]

@kingdaro

Comments on folder structure? Specifically, where index.d.ts should go, and where the typings tests should go

Put stuff related to each other by each other. Just like index.js and index.test.js are siblings, so I imagine src/index.d.ts.

But really I don't care, wherever you'd put it is fine.

[itsMapleLeaf]

Here's what I settled on for typing the route component props:

export type RouteComponentProps<TParams = {}> = Partial<TParams> & {
  path: string;
  default?: boolean;
  location?: WindowLocation;
  navigate?: NavigateFn;
  uri?: string;
};

import * as React from "react";
import { render } from "react-dom";
import { RouteComponentProps, Router } from "../";

type DashParams = { id: string };

let Home = (props: RouteComponentProps) => <div>Home</div>;

let Dash = (props: RouteComponentProps<DashParams>) => (
  <div>Dash for item ${props.!id}</div>
);

render(
  <Router>
    <Home path="/" />
    <Dash path="/dashboard/:id" />
  </Router>,
  document.getElementById("app-root")
);

I merged them all into one props interface, as suggested before, and I made all of its receiving props optional so they don't need to be provided when using them in <Router />. Same reason for the Partial<TParams>, so the params don't need to be passed in.

Not completely ideal, since either undefined checks or a null assertion ! is required in the component body to use the uri, params and such, but it's the simplest solution I've come up with so far that didn't have some hitch in implementation. Just to recap on everything else I've considered:

• The approach I went with first (casting SomeComponent as React.SFC<RouteComponentMatchingProps>) only seems to work if SomeComponent doesn't accept any other props, otherwise this throws an error due the incompatibility between SomeComponent's props and RouteComponentMatchingProps
• Using module augmentation to add path?: string to element props globally is not ideal. The types should restrict the path property only to what can actually accept it, and not allow it on every element.
• Changing the type of React.createElement doesn't seem to work, most likely due to the way JSX elements lose their typing information when created. As far as I know, strictly typing JSX children isn't possible at all at the moment (e.g. we can restrict to just elements, but not to specific kinds of elements)

With that in mind, this is probably good enough, just so we can get some types out there and make refinements later if needed.

[joshthecoder]

@kingdaro Looking good. I'll give this a test run and report back if I run into any issues. Nice work

[antmdvs]

I tried to enforce this invariant, but couldn't get it working. Maybe you guys could take a stab at it?

[itsMapleLeaf]

@antmdvs I was able to enforce either path or default on RouteComponentProps if that's what you meant, though there's no way to check that just on the children of Router.

Though I recall the react-transition-group package doing something like that, the error messages I've gotten from using it are a little cryptic, and I don't think it would be worth it.

[antmdvs]

@kingdaro It looks like your last change breaks IntelliSense, though. The path and default props no longer appear. Not sure if this might help.

Anyway, to clarify what I meant, I was trying to do something like this in the Router def:

children: React.ReactElement<{ default: boolean } | { path: string } | Redirect>

[itsMapleLeaf]

It looks like your last change breaks IntelliSense, though. The path and default props no longer appear. Not sure if this might help.

I attempted to do it the way the linked answer does (with extending interfaces), but the props still don't show up on autocomplete. As far as I can tell, it's no different than what I'd done already.

If preserving autocomplete is a priority, then making path and default both optional properties on the props is another option, but then TS wouldn't error if you left both of them off. More tradeoffs ¯\_(ツ)_/¯

That sounds like a fine solution to me, though. Reach Router will still error at runtime if one forgets to define a path or default.

Anyway, to clarify what I meant, I was trying to do something like this in the Router def:

Right, and that unfortunately doesn't work, because JSX elements lose their typing information in usage. Elements collapse to JSX.Element which is ReactElement<any>, and is assignable to any ReactElement<...> type. As far as I know, there's no real way to do it without extra strict typings from the library user - something I imagine we'd like to avoid wherever possible.

[antmdvs]

Tough one.. FWIW, I'd personally favor having them in the autocomplete list and letting the runtime error handle the validation. I use autocomplete heavily when exploring a new API and it would seem wrong if a component accepted props (without TypeScript complaining) that don't appear in the autocomplete list.

Right, and that unfortunately doesn't work, because JSX elements lose their typing information in usage. Elements collapse to JSX.Element which is ReactElement, and is assignable to any ReactElement<...> type.

Weird. I just find that super confusing because of this and this. Oh well.

[itsMapleLeaf]

Yeah, I rely on autocomplete pretty heavily myself, and I imagine it's not too different for the rest of the TS ecosystem. I'll opt for the optional props then.

[sseppola]

I copied your definition file to run it locally until this module is ready. One thing I had to change was your LinkProps so it accepts target amongst others by using React.DetailedHTMLProps<React.AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement> instead of React.HTMLAttributes<HTMLAnchorElement>.

Additionally I omitted the href prop because it will be ignored, so I'd rather TS give a warning about using href.

Here's my code:

  type Omit<T, K> = Pick<T, Exclude<keyof T, K>>;
  type AnchorProps = Omit<
    React.DetailedHTMLProps<
      React.AnchorHTMLAttributes<HTMLAnchorElement>,
      HTMLAnchorElement
    >,
    'href'
  >;

  export interface LinkProps<TState> extends AnchorProps {
    to?: string;
    replace?: boolean;
    getProps?: (props: LinkGetProps) => {};
    state?: TState;
  }

Thanks for the work on the definitions 👍

[itsMapleLeaf]

Thanks for the patch, I'll go ahead and throw that in

[antmdvs]

I just have a question. Exclude is a TS 2.8 thing. Does it matter what version of TS is used here from a consumer standpoint? I've never authored a lib, so I'm not sure how that works. IOW, would TS 2.8 (or whatever the lowest version that's compatible with this d.ts) be a peer dep or something? Thanks.

[itsMapleLeaf]

I'm not 100% sure on that myself, but I imagine a peer dependency wouldn't be the best idea, as it would show a warning for everyone who installs the library to install TS, including those who don't use TS.

Good to point that out, though. It seemed reasonable to rely on people having their TS version updated, but that might not always be the case, and an error on Exclude isn't really a good "you're outdated" message.

Maybe a simple // requires TypeScript 2.8+ comment above the Omit declaration will do? So when someone gets the error, they'll see it and know that they have to upgrade.

That, or we could remove the usage of Omit altogether and leave the href prop as-is, though that could harm DX if someone attempts to use it when it gets ignored by the router.

A few options here. Leaning towards the upgrade comment.

[antmdvs]

...it would show a warning for everyone who installs the library to install TS, including those who don't use TS.

So things brings up another Q: If reach router shipped a @types package instead of a bundled index.d.ts, non-TS users wouldn't be affected. Do you think that would be preferable?

[agundermann]

Another thing to consider is that if reach router bundles index.d.ts, those definitions need to be always kept up to date. Otherwise it's quite inconvenient for consumers to add their own definitions after a breaking change. With @ types, it would be easier to include some (albeit suboptimal) type definitions via declare module until the official types get updated.

I faced this problem recently with redux-thunk (see reduxjs/redux-thunk#169). I think it's also why the ts docs recommend using @ types for non-ts packages.

[itsMapleLeaf]

Hm... all things considered, hitting up DefinitelyTyped is starting to sound like the better option. I guess it wouldn't be too much trouble to move the typedefs over there. I'll probably get around to that this evening, then.

Closing for now

[itsMapleLeaf]

DefinitelyTyped/DefinitelyTyped#26276