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

Instantiating and type-checking a generic React component #7145

Closed
samstarling opened this Issue Nov 7, 2018 · 13 comments

Comments

Projects
None yet
2 participants
@samstarling
Copy link

samstarling commented Nov 7, 2018

Is there a good way to type-check a generic React component? Here's an example:

// @flow
import * as React from 'react';

type GenericListProps<T> = {
  renderItem: (item: T) => React.Node,
  items: Array<T>,
};

class GenericList<T> extends React.Component<GenericListProps<T>> {
  render() {
    return this.props.items.map(this.props.renderItem);
  }
}

export default class StringList extends React.Component<*> {
  render() {
    return (
      <GenericList
        items={['A', 2, 'B', 'C']}
        renderItem={(str) => <p>{str}</p>}
      />
    );
  }
}

In this case, I want to somehow instantiate a GenericList<String> within the StringList component. Is there a good way to do this? (Imagine in the future I have a NumberList, and an ImageList, and so. on).

@wchargin

This comment has been minimized.

Copy link
Contributor

wchargin commented Nov 7, 2018

If I understand correctly, you want your render function to raise a
type error because 2 is not a string. But it currently is not because
Flow (correctly) infers that you can use GenericList<string | number>
in your example. Is this correct?

You could explicitly refine the component type:

  render() {
    const GL: React.ComponentType<GenericListProps<string>> = GenericList;
    return <GL items={["A", 2, "B", "C"]} renderItem={(str) => <p>{str}</p>} />;
  }

Or, you could restrict the inference by annotating the function
parameter type:

  render() {
    return (
      <GenericList
        items={["A", 2, "B", "C"]}
        renderItem={(str: string) => <p>{str}</p>}
      />
    );
  }

Either of these raises a type error.

@samstarling

This comment has been minimized.

Copy link

samstarling commented Nov 7, 2018

@wchargin It's more that I'd like to be able to create a StringList component that contains a GenericList<string>, and I think your first example achieves that... Let me have a go and see how it works. Does it feel like I'm trying to do something unusual?

@samstarling

This comment has been minimized.

Copy link

samstarling commented Nov 7, 2018

I was also wondering if there was a way to do it without having to export Props<T> from my GenericList<T> file (the example from above was simplified, as an example).

@wchargin

This comment has been minimized.

Copy link
Contributor

wchargin commented Nov 7, 2018

Yes, you can certainly create a StringList component that contains a
GenericList<string>. This is not unusual.

In your first example, you create a GenericList<string | number>. This
is also fine.

It seems to me like you’re trying to do something reasonable, you’re
doing it correctly, and Flow is typechecking it correctly. Could you
indicate what Flow is doing that you think it should not be doing?

It might help to flesh out your example a bit, such that StringList
doesn’t just render a constant set of items. For instance, if your
renderItem were a prop (s: string) => Node, then the example would
behave differently.

I was also wondering if there was a way to do it without having to
export Props<T> from my GenericList<T> file

You shouldn’t have to export Props<T> at all, no. Your example works
fine if you do not.

@samstarling

This comment has been minimized.

Copy link

samstarling commented Nov 9, 2018

@wchargin Thanks for the reply, I appreciate it. I put together an example project here. Hopefully that's a little more helpful than the example I gave before – I tried to put both of your examples into practice here, and I hope I understood them correctly!

Sadly, flow does not succeed in type-checking the examples:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/App.js:13:56

Cannot create GenericList element because number [1] is incompatible with T [2] in the first argument of property
renderItem.

     src/App.js
     10│     return (
     11│       <div className="App">
     12│         <h1>First Generic List</h1>
 [1] 13│         <GenericList items={[1, 2, 3]} renderItem={(n: number) => n} />
     14│         <h1>Second Generic List</h1>
     15│         <SecondGenericList items={[1, 2, 3]} renderItem={(n: number) => n} />
     16│       </div>

     src/components/GenericList.js
 [2]  6│   renderItem: <T>(item: T) => React.Node


Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/App.js:15:62

Cannot create SecondGenericList element because number [1] is incompatible with T [2] in the first argument of property
renderItem.

     src/App.js
     12│         <h1>First Generic List</h1>
     13│         <GenericList items={[1, 2, 3]} renderItem={(n: number) => n} />
     14│         <h1>Second Generic List</h1>
 [1] 15│         <SecondGenericList items={[1, 2, 3]} renderItem={(n: number) => n} />
     16│       </div>
     17│     );
     18│   }

     src/components/GenericList.js
 [2]  6│   renderItem: <T>(item: T) => React.Node
@wchargin

This comment has been minimized.

Copy link
Contributor

wchargin commented Nov 9, 2018

Thanks for the example! I see now what you mean about exporting the
Props type (I didn’t understand before that you wanted these to be in
separate modules). You can avoid exporting Props by instead exporting

// GenericList.js
export type GenericListComponentType<T> = React.ComponentType<Props<T>>;

and then using

const SecondGenericList: GenericListComponentType<number> = GenericList;

in the client. (It’s the same thing, of course, just hiding the
implementation details.)


The reason that your module does not typecheck is that you have defined
your props as

export type Props<T> = {
  items: Array<T>,
  renderItem: <T>(item: T) => React.Node
};

where you meant to write

export type Props<T> = {
  items: Array<T>,
  renderItem: (item: T) => React.Node
};

The former is equivalent to

export type Props<T> = {
  items: Array<T>,
  renderItem: <S>(item: S) => React.Node
};

which is not what you want: it means that the renderItem function must
be able to render any type T (or S), not just the T that was
already declared above. Writing <T>(item: T) => … introduces a new
type parameter T that shadows the outer parameter.

Does this make sense?

@samstarling

This comment has been minimized.

Copy link

samstarling commented Nov 9, 2018

@wchargin Aha, this does make a lot of sense – thanks for pointing out my misunderstanding, that does indeed help. I just updated the example, and I still get left with one slightly different error:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/components/GenericList.js:11:39

Cannot call this.props.renderItem with item bound to item because T [1] is incompatible with T [2].

      8│
 [2]  9│ export default class GenericList<T> extends React.Component<Props<T>> {
 [1] 10│   renderListItem = <T>(item: T) => {
     11│     return <li>{this.props.renderItem(item)}</li>;
     12│   };
     13│
     14│   render() {

Is that also down to the <T>(item: T) definition causing the same problem? If I remove the <T>, I get a similar error but within the render() method:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/components/GenericList.js:15:38

Cannot call this.props.items.map with this.renderListItem bound to callbackfn because T [1] is incompatible with T [2]
in the first argument.

 [2]  5│   items: Array<T>,
       :
 [1] 10│   renderListItem = (item: T) => {
     11│     return <li>{this.props.renderItem(item)}</li>;
     12│   };
     13│
     14│   render() {
     15│     return <ul>{this.props.items.map(this.renderListItem)}</ul>;
     16│   }
     17│ }
     18│
@wchargin

This comment has been minimized.

Copy link
Contributor

wchargin commented Nov 9, 2018

Yes, sorry, forgot to mention that (fixed it by muscle memory in my
local copy :-) ). You are correct that the <T> is shadowing in the
same way and that you should remove it. But you also need to explicitly
annotate the type of the field:

export default class GenericList<T> extends React.Component<Props<T>> {
  renderListItem: T => React.Node = (item: T) => {
    return <li>{this.props.renderItem(item)}</li>;
  };

  render() {
    return <ul>{this.props.items.map(this.renderListItem)}</ul>;
  }
}

This is #4743, et al. (There are at least a few more duplicate issues of
this bug, but I can’t find them all right now.)

@samstarling

This comment has been minimized.

Copy link

samstarling commented Nov 9, 2018

Perfect! That makes sense now. I also realise that perhaps T => React.Node is a nicer/shorter type to have in my props as well, rather than explicitly having (item: T) => React.Node. Once the bug behind #4743 is fixed, would renderListItem = (item: T) => ... work correctly?

@samstarling

This comment has been minimized.

Copy link

samstarling commented Nov 9, 2018

@wchargin One other final thing I wanted to ask: would it be at all possible to put bounds on the type T? For example, to ensure that T is always an object with a key attribute?

@wchargin

This comment has been minimized.

Copy link
Contributor

wchargin commented Nov 9, 2018

perhaps T => React.Node is a nicer/shorter type to have in my props
as well, rather than explicitly having (item: T) => React.Node

Yeah—they’re equivalent, of course, but I tend to leave out superfluous
names on parameters and indexers, like (number: number) => string or
{[objectId: ObjectId]: Datum}.

Once the bug behind #4743 is fixed, would renderListItem = (item: T) => ...
work correctly?

It should, yes.

would it be at all possible to put bounds on the type T? For
example, to ensure that T is always an object with a key
attribute?

Yep:

class GenericList<T: {+key: string}> extends React.Component<GenericListProps<T>> {
  //
}

will require that T be a subtype of {+key: string}, which is true as
long as it is an object type with a readable property key of type some
subtype of string.

@samstarling

This comment has been minimized.

Copy link

samstarling commented Nov 10, 2018

Perfect! Thanks for all the help @wchargin – I updated my blog post and gave you some thanks at the bottom, because the solution feels a lot cleaner and nicer now. I'll close this issue 😄

@wchargin

This comment has been minimized.

Copy link
Contributor

wchargin commented Nov 11, 2018

You're quite welcome; I'm glad that this helped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment