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

@types/react-router: Can't pass Components into react-router Routes with custom type arguments defined #13689

Open
fultonm opened this Issue Jan 2, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@fultonm

fultonm commented Jan 2, 2017

  • I tried using the latest xxxx/xxxx.d.ts file in this repo and had problems.
  • I tried using the latest stable version of tsc. https://www.npmjs.com/package/typescript
  • I have a question that is inappropriate for StackOverflow. (Please ask any appropriate questions there).
  • [x ] I want to talk about xxxx/xxxx.d.ts.

ReactTraining/react-router#4317

I opened an issue on the react-router repo regarding a problem with react-router and Typescript. I'm not sure which issue tracker it belongs to, so maybe someone can take a look and see if the bug is more appropriate for this repo?

@Izhaki

This comment has been minimized.

Izhaki commented Mar 27, 2017

I'm don't think that there's an issue with @types here.

If you look at the following line (interface RouteProps):

component?: React.SFC<RouteComponentProps<any> | void> | React.ComponentClass<RouteComponentProps<any> | void>;

it basically gives you two options:

  • Don't use typings at all. That's why React.Component<any, any> works.
  • Strictly adhere to typings.

For the latter case, your props interface should extend RouteComponentProps like so:

export interface EntitiesPageProps extends RouteComponentProps<any> { }

and the component as usual:

export class EntitiesPage extends React.Component< EntitiesPageProps, undefined >  { }

What you provide in ReactTraining/react-router#4317 is neither the first nor the second option - you do provide a type, but not one that adheres to the router typings.

@Izhaki

This comment has been minimized.

Izhaki commented Mar 28, 2017

Another working solution is to simply use @types/react-router-redux instead of @types/react-router. See this project as an example.

@mikebridge

This comment has been minimized.

mikebridge commented Apr 21, 2017

@fultonm The TypeScript looks correct—the problem is that the Route component injects several variables into the wrapped component. Maybe there's a good reason for that design but I can't see why all those props are necessary. I thought that was the purpose of withRouter. Anyway, I think it's better to wrap the subcomponents like this so they don't get polluted with unnecessary dependencies:

import * as React from "react";
import {RouteComponentProps} from "react-router";

export function withoutRouting<P>(Component: React.ComponentClass<P> | React.SFC<P> ):
    React.ComponentClass<RouteComponentProps<P>> {

    class C extends React.Component<any, any> {

        public render(): JSX.Element {

            const {match, location, history, staticContext, ...rest} = this.props;
            return (
                <Component {...rest} />
            );
        }
    }
    return C;
}

export default withoutRouting;

Then:

    <Route path="/test" component={withoutRouting(MyComponent)} />
@sbuzonas

This comment has been minimized.

sbuzonas commented Jun 11, 2017

The definition does appear to be correct. The snippet directly above will exclude the props entirely which is the better solution IMO.

Wrapping everything in withoutRouting didn't feel right to me, so I took it a step farther and created a wrapper for the Route component.

import * as React from 'react';
import { Route as BaseRoute, RouteComponentProps, RouteProps } from 'react-router';

export function withoutRouteProps<P>(
  Component: React.ComponentClass<P> | React.SFC<P>
): React.ComponentClass<RouteComponentProps<P>> {
  class C extends React.Component<any, any> {
    public render(): JSX.Element {
      const {match, location, history, staticContext, ...componentProps} = this.props;
      return <Component {...componentProps} />;
    }
  }
  return C;
}

interface FilteredRouteProps extends RouteProps {
  component?: React.SFC<any> | React.ComponentClass<any>;
}

export function Route(props: FilteredRouteProps) {
  const {
    component,
    ...routeProps
  } = props;

  if (component) {
    return <BaseRoute component={withoutRouteProps(component)} {...routeProps} />;
  }

  return <BaseRoute {...routeProps} />;
}
@sbuzonas

This comment has been minimized.

sbuzonas commented Jun 11, 2017

Although on second glance. Isn't it forcing the props to be only RouteComponentProps<any>.

I would think it would be better suited as ComponentProps & Partial<RouteComponentProps<any>>?

@mikebridge

This comment has been minimized.

mikebridge commented Jun 11, 2017

@sbuzonas That seems like a good idea---I was looking at the RouteComponentProps<any> thing and ran into some Liskov issues when overriding component, so I added a new property, plaincomponent:

import * as React from "react";

import {Route, RouteProps} from "react-router";

import {withoutRouteProps} from "./withoutRouteProps";

interface IFilteredRouteComponentProps<P> {
    plaincomponent?: React.SFC<P> | React.ComponentClass<P>;
}

export class FilteredRoute<P> extends React.Component<RouteProps & IFilteredRouteComponentProps<P>, {}> {

    public render(): JSX.Element {
        const {plaincomponent, component, ...rest} = this.props;

        return plaincomponent ? (
            <Route component={withoutRouteProps(plaincomponent)} {...rest} />
        ) : (
            <Route component={component} {...rest} />
        );
    }
}

export default FilteredRoute;

I also modified the types on withoutRouting / withoutRouteProps slightly to get rid of any:

import * as React from "react";
import {RouteComponentProps} from "react-router";

export function withoutRouteProps<P>(Component: React.ComponentClass<P> | React.SFC<P> ):

React.ComponentClass<RouteComponentProps<P>> {

    class C extends React.Component<RouteComponentProps<P>, {}> {

        public render(): JSX.Element {

            const {match, location, history, staticContext, ...rest} = this.props;
            return (
                <Component {...rest} />
            );
        }
    }
    return C;
}

export default withoutRouteProps;

So:

<FilteredRoute path="/test" plaincomponent={MyComponentThatDoesNotKnowAboutRouting} />
<FilteredRoute path="/test" component={MyComponentThatImplementsRouting} />

@ulrikstrid ulrikstrid referenced this issue Jun 16, 2017

Closed

react-router needs more props #17086

2 of 15 tasks complete
@sbuzonas

This comment has been minimized.

sbuzonas commented Jul 5, 2017

@mikebridge My RouteComponentProps<any> statement was IRT

component?: React.SFC<RouteComponentProps<any> | undefined> | React.ComponentClass<RouteComponentProps<any> | undefined>;
render?: ((props: RouteComponentProps<any>) => React.ReactNode);
children?: ((props: RouteComponentProps<any>) => React.ReactNode) | React.ReactNode;

Shouldn't that be an intersection, rather than only route props or no props, like:

type RouteComponentPropsIntersection<P> = RouteContextProps<any> & P;

export interface RouteProps<P> {
  location?: H.Location;
  component?: React.SFC<RouteComponentPropsIntersection<P> | undefined> | React.ComponentClass<RouteComponentPropsIntersection<P> | undefined>;
  render?: ((props: RouteComponentProps<any>) => React.ReactNode);
  children?: ((props: RouteComponentProps<any>) => React.ReactNode) | React.ReactNode;
  path?: string;
  exact?: boolean;
  strict?: boolean;
}
export class Route extends React.Component<RouteProps<{}>> { }
export function matchPath<P>(pathname: string, props: RouteProps<{}>): match<P> | null;

sbuzonas added a commit to sbuzonas/DefinitelyTyped that referenced this issue Jul 5, 2017

react-router: Add union to component properties
This is a different approach to DefinitelyTyped#17086 it solves the issue in DefinitelyTyped#13689

All components that are children of Route are passed the RouteComponentProps, this is typed
as a partial because not all components are router aware, and do not declare/use the props.

Components also extend a generic class that takes properties and state types, these changes
allow for inclusion of the user defined types with a union of the RouteComponentProps provided
by the react-router library.
@mikebridge

This comment has been minimized.

mikebridge commented Jul 5, 2017

@sbuzonas Looking at this again I think you're probably correct that there's something wrong with that type---I need to look at this thing more closely again. In the meantime I had to stop using this workaround because someone here noticed that it was causing extra remounting. So currently all my Routed components' props are intersected with RouteComponentProps<any>.

@sbuzonas

This comment has been minimized.

sbuzonas commented Jul 5, 2017

@mikebridge I created a PR the eliminates the need for a workaround in my scenarios.

sbuzonas added a commit to sbuzonas/DefinitelyTyped that referenced this issue Jul 15, 2017

react-router: Add union to component properties
This is a different approach to #17086 it solves the issue in #13689

All components that are children of Route are passed the RouteComponentProps, this is typed
as a partial because not all components are router aware, and do not declare/use the props.

Components also extend a generic class that takes properties and state types, these changes
allow for inclusion of the user defined types with a union of the RouteComponentProps provided
by the react-router library.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment