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

Can't use PropsWithChildren on route components #650

Closed
nirvdrum opened this issue Jan 7, 2020 · 5 comments
Closed

Can't use PropsWithChildren on route components #650

nirvdrum opened this issue Jan 7, 2020 · 5 comments

Comments

@nirvdrum
Copy link

nirvdrum commented Jan 7, 2020

I may be doing something wrong, but I've tried to write a functional component for the router that accepts the router state from found and provides access to the children:

import React, { PropsWithChildren } from 'react';

export function PageLayout(props: PropsWithChildren<RouterState>) {
  return props.children;
}

Unfortunately, that raises the following TypeScript error:

TypeScript error in /home/nirvdrum/dev/workspaces/cloudtruth/ctweb/src/RelayRouter/Routes.tsx(31,10):
No overload matches this call.
  Overload 1 of 2, '(props: Readonly<RouteProps>): Route', gave the following error.
    Type '(props: PropsWithChildren<RouterState>) => ReactNode' is not assignable to type 'ComponentClass<any, any> | FunctionComponent<any> | undefined'.
      Type '(props: PropsWithChildren<RouterState>) => ReactNode' is not assignable to type 'FunctionComponent<any>'.
        Type 'ReactNode' is not assignable to type 'ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<any, any, any>)> | null'.
          Type 'undefined' is not assignable to type 'ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<any, any, any>)> | null'.
  Overload 2 of 2, '(props: RouteProps, context?: any): Route', gave the following error.
    Type '(props: PropsWithChildren<RouterState>) => ReactNode' is not assignable to type 'ComponentClass<any, any> | FunctionComponent<any> | undefined'.
      Type '(props: PropsWithChildren<RouterState>) => ReactNode' is not assignable to type 'FunctionComponent<any>'.  TS2769

    29 | 
    30 | export const Routes = makeRouteConfig(
  > 31 |   <Route Component={PageLayout}>
       |          ^

I'm still fairly new to both TypeScript and React, so maybe I'm just overlooking something. But, I don't see how my component doesn't adhere to found's type signature. Following on from that, I don't see a way to make my component match the type signature.

My solution for the time being is to replace PropsWithChildren with a new interface defining my props like so:

interface Props {
  children: any
  match: Match
  router: Router
}

This approach works fine*. TypeScript is happy and I can access the fields I'd like. It'd be nice to be able to use PropsWithChildren, however, just to maintain consistency in my code.

  • Note that I had to change the type of the children field; if I mark it ReactNode, this interface will result in a similar error.
@taion
Copy link
Contributor

taion commented Jan 7, 2020

I believe your solution with defining interface Props is actually the correct one – at least, it's what we do.

I think it's nicer than PropsWithChildren because it makes it immediately clear what the props actually are within the context of the component. At least, it matches other components.

One minor quibble – the typing on children should probably be React.ReactNode. Maybe.

@nirvdrum
Copy link
Author

nirvdrum commented Jan 7, 2020

Thanks for the response. I was under the impression PropsWithChildren was the idiomatic way to access children with defined props. But, I also was looking to use a defined type from found for the component props so I'm never at risk of my custom interface falling out-of-date with what found expects. I thought RouterState was that props definition, but it doesn't have access to the component's children, thus this whole problem.

One minor quibble – the typing on children should probably be React.ReactNode. Maybe.

I have a footnote in my comment speaking to that. If I type children as React.ReactNode, I end up with the same error I get when I use PropsWithChildren. I don't know what it is, but something in the type definition does not like children being React.ReactNode. If that worked, then PropsWithChildren should work as well, since that's defined as type PropsWithChildren<P> = P & { children?: ReactNode };

@taion
Copy link
Contributor

taion commented Jan 7, 2020

With children: React.ReactNode, the type error is because, for whatever reason, the TS React types aren't okay with returning generic React nodes, and instead expects a React element or something else. See DefinitelyTyped/DefinitelyTyped#18051. For now, you can return <>props.children</>.

Actually, that's what's making your initial example not work as well, now that I look at the error. The type we have is "any component type" anyway:

found/types/index.d.ts

Lines 280 to 283 in e24d9cc

/**
* the component for the route
*/
Component?: React.ComponentType<any>;
.

There's no need to use any defined type here, because Found doesn't expect anything of the component. It just provides props to the component, but that's not represented in the typings here, even.

@nirvdrum
Copy link
Author

nirvdrum commented Jan 7, 2020

Thanks for clarifying. My confusion around the types stemmed from found using any. I couldn't see how what I was doing wouldn't conform to the type signature. So I thought maybe this was some weird manifestation of #404 or #646. Sorry for wasting your time. I tried to be pretty diligent about tracking this down before filing the issue and thought I had tried wrapping children, but apparently did not. In any event, thank you for setting me straight.

@nirvdrum nirvdrum closed this as completed Jan 7, 2020
@taion
Copy link
Contributor

taion commented Jan 7, 2020

No worries. Good luck!

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

2 participants