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

Remove React.FC from Typescript template #8177

Merged
merged 1 commit into from Jan 22, 2020
Merged

Conversation

Retsam
Copy link
Contributor

@Retsam Retsam commented Dec 13, 2019

This removes React.FC from the base template for a Typescript project.

Long explanation for a small change:

React.FC is unnecessary: it provides next to no benefits and has a few downsides. (See below.) I see a lot of beginners to TS+React using it, however, and I think that it's usage in this template is a contributing factor to that, as the prominence of this template makes it a de facto source of "best practice".

Downsides to React.FC/React.FunctionComponent

Provides an implicit definition of children

Defining a component with React.FC causes it to implicitly take children (of type ReactNode). It means that all components accept children, even if they're not supposed to, allowing code like:

const App: React.FC = () => { /*... */ };
const Example = () => {
	<App><div>Unwanted children</div></App>
}

This isn't a run-time error, but it is a mistake and one that would be caught by Typescript if not for React.FC.

Doesn't support generics.

I can define a generic component like:

type GenericComponentProps<T> = {
   prop: T
   callback: (t: T) => void
}
const GenericComponent = <T>(props: GenericComponentProps<T>) => {/*...*/}

But it's not possible when using React.FC - there's no way to preserve the unresolved generic T in the type returned by React.FC.

const GenericComponent: React.FC</* ??? */> = <T>(props: GenericComponentProps<T>) => {/*...*/}
Makes "component as namespace pattern" more awkward.

It's a somewhat popular pattern to use a component as a namespace for related components (usually children):

<Select>
	<Select.Item />
</Select>

This is possible, but awkward, with React.FC:

const  Select: React.FC<SelectProps> & { Item: React.FC<ItemProps> } = (props) => {/* ... */ }
Select.Item = (props) => { /*...*/ }

but "just works" without React.FC:

const Select = (props: SelectProps) => {/* ... */}
Select.Item = (props: ItemProps) => { /*...*/ }
Doesn't work correctly with defaultProps

This is a fairly moot point as in both cases it's probably better to use ES6 default arguments, but...

type  ComponentProps = { name: string; }

const  Component = ({ name }: ComponentProps) => (<div>
	{name.toUpperCase()} /* Safe since name is required */
</div>);
Component.defaultProps = { name: "John" };

const  Example = () => (<Component />) /* Safe to omit since name has a default value */

This compiles correctly. Any approach with React.FC will be slightly wrong: either React.FC<{name: string}> will make the prop required by consumers, when it should be optional, or React.FC<{name?: string}> will cause name.toUpperCase() to be a type error. There's no way to replicate the "internally required, externally optional" behavior which is desired.

It's as long, or longer than the alternative: (especially longer if you use FunctionalComponent):

Not a huge point, but it isn't even shorter to use React.FC

const C1: React.FC<CProps> = (props) => { }
const C2 = (props: CProps) => {};

Benefits of React.FC

Provides an explicit return type

The only benefit I really see to React.FC (unless you think that implicit children is a good thing) is that it specifies the return type, which catches mistakes like:

const Component = () => {
   return undefined; // components aren't allowed to return undefined, just `null`
}

In practice, I think this is fine, as it'll be caught as soon as you try to use it:

const Example = () => <Component />; // Error here, due to Component returning the wrong thing

But even with explicit type annotations, React.FC still isn't saving very much boilerplate:

const Component1 = (props: ComponentProps): JSX.Element => { /*...*/ }
const Component2: FC<ComponentProps> = (props) => { /*...*/ }

This removes `React.FC` from the base template for a Typescript project.

Long explanation for a small change: 

`React.FC` is unnecessary: it provides next to no benefits and has a few downsides.  (See below.)  I see a lot of beginners to TS+React using it, however, and I think that it's usage in this template is a contributing factor to that, as the prominence of this template makes it a de facto source of "best practice".  

### Downsides to React.FC/React.FunctionComponent

##### Provides an implicit definition of `children`

Defining a component with `React.FC` causes it to implicitly take `children` (of type `ReactNode`).  It means that all components accept children, even if they're not supposed to, allowing code like:

```ts
const App: React.FC = () => { /*... */ };
const Example = () => {
	<App><div>Unwanted children</div></App>
}
```

This isn't a run-time error, but it is a mistake and one that would be caught by Typescript if not for `React.FC`. 

##### Doesn't support generics.
I can define a generic component like:
```ts
type GenericComponentProps<T> = {
   prop: T
   callback: (t: T) => void
}
const GenericComponent = <T>(props: GenericComponentProps<T>) => {/*...*/}
```

But it's not possible when using `React.FC` - there's no way to preserve the unresolved generic `T` in the type returned by `React.FC`.

```ts
const GenericComponent: React.FC</* ??? */> = <T>(props: GenericComponentProps<T>) => {/*...*/}
```

##### Makes "component as namespace pattern" more awkward.
It's a somewhat popular pattern to use a component as a namespace for related components (usually children):

```jsx
<Select>
	<Select.Item />
</Select>
```

This is possible, but awkward, with `React.FC`:

```tsx
const  Select: React.FC<SelectProps> & { Item: React.FC<ItemProps> } = (props) => {/* ... */ }
Select.Item = (props) => { /*...*/ }
```

but "just works" without `React.FC`:

```tsx
const Select = (props: SelectProps) => {/* ... */}
Select.Item = (props) => { /*...*/ }
```

##### Doesn't work correctly with defaultProps

This is a fairly moot point as in both cases it's probably better to use ES6 default arguments, but...

```tsx
type  ComponentProps = { name: string; }

const  Component = ({ name }: ComponentProps) => (<div>
	{name.toUpperCase()} /* Safe since name is required */
</div>);
Component.defaultProps = { name: "John" };

const  Example = () => (<Component />) /* Safe to omit since name has a default value */
```
This compiles correctly.  Any approach with `React.FC` will be slightly wrong: either `React.FC<{name: string}>` will make the prop required by consumers, when it should be optional, or `React.FC<{name?: string}>` will cause `name.toUpperCase()` to be a type error.  There's no way to replicate the "internally required, externally optional" behavior which is desired.

##### It's as long, or longer than the alternative: (especially longer if you use `FunctionalComponent`):
Not a huge point, but it isn't even shorter to use `React.FC` 
```ts
const C1: React.FC<CProps> = (props) => { }
const C2 = (props: CProps) => {};
```

### Benefits of React.FC

##### Provides an explicit return type

The only benefit I really see to `React.FC` (unless you think that implicit `children` is a good thing) is that it specifies the return type, which catches mistakes like:

```ts
const Component = () => {
   return undefined; // components aren't allowed to return undefined, just `null`
}
```

In practice, I think this is fine, as it'll be caught as soon as you try to use it:

```ts
const Example = () => <Component />; // Error here, due to Component returning the wrong thing
```

But even with explicit type annotations, `React.FC` still isn't saving very much boilerplate:

```ts
const Component1 = (props: ComponentProps): ReactNode => { /*...*/ }
const Component2: FC<ComponentProps> = (props) => { /*...*/ }
```
@facebook-github-bot
Copy link

Hi Retsam! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Copy link
Contributor

@heyimalex heyimalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 15, 2019

@ianschmitz I know you work with CRA and TypeScript too, what are your thoughts on this?

I can see both sides. For beginners, it's nice to show them that this is available - but I can see how it could be confusing that it implies that children are used, especially when used with the App component.

@ianschmitz ianschmitz added this to the 3.3.1 milestone Dec 16, 2019
@ianschmitz
Copy link
Contributor

ianschmitz commented Dec 16, 2019

My only hesitation was for new comers that get stuck on how they should type the children prop. Hopefully we can lean on this section of the documentation to point people in the right direction: https://create-react-app.dev/docs/adding-typescript/#getting-started-with-typescript-and-react.

I don't see propTypes used much with TS, and defaultProps can also be accomplished with ES default parameters, so there's not much lost beyond the children type, and it's easy enough to use PropsWithChildren directly as well.

@Retsam thanks for the detailed write-up - much appreciated.

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 18, 2019

@ianschmitz We talked about adding a second component to the template, maybe that would solve the issue?

@josias-r
Copy link

I'm confused about JSX.Element vs ReactNode. React.createElement will return a JSX Element. Why do you declare the return type to be ReactNode? (Why would you even declare it at all, it should automatically detect the return type when returning an element)

@Retsam
Copy link
Contributor Author

Retsam commented Jan 21, 2020

@josias-r Ah, it looks like I was wrong on this point. ReactNode is the broader type for things that are valid for React to render (e.g. string, null). In theory, I think it's the right return type for react components, but there's some issues with using it as the return type in practice. There's a bit of discussion on this react-typescript-cheatsheet issue.

Specifically, I thought annotating : ReactNode would allow something like this:

const SometimesFancyButton = ({text, fancy}: SometimesFancyButtonProps) => {
    return fancy ? (<span className="fancy">{text}</span>) : text;
};

But it looks like it's just not possible to make the compiler happy with that, without a cast. (And React.FC doesn't support that, either.) I'll edit my example to annotate as JSX.Element instead of ReactNode.


(Why would you even declare it at all, it should automatically detect the return type when returning an element)

I agree, and I don't annotate my function components return types. (Hence why I was wrong about the correct annotation...)

But some developers/teams insist on annotating all return types as a style, often enforced by @typescript-eslint/explicit-function-return-type rule. I'm not a big fan of that rule - but it's part of the typescript-eslint recommended ruleset.

@josias-r
Copy link

@Retsam Ah okay, thank you for the clarification! 🙏

@ELI7VH
Copy link

ELI7VH commented Jan 22, 2020

I think I prefer implicit children, if not only for having the type of the function and the props declared together, I have always hated the const Thing = ({ foo, bar }: Props) => { look. the Props inside the parens always goofs with my eyes.

@ianschmitz
Copy link
Contributor

@Retsam ready to go?

@Retsam
Copy link
Contributor Author

Retsam commented Jan 22, 2020

@ianschmitz Good to go on my end.

@ianschmitz ianschmitz merged commit dada035 into facebook:master Jan 22, 2020
@lock lock bot locked and limited conversation to collaborators Jan 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants