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

FunctionComponent and ComponentClass are not compatible with LibraryManagedAttributes #87

Closed
ferdaber opened this issue Feb 14, 2019 · 19 comments

Comments

@ferdaber
Copy link
Collaborator

Annotating functions and classes with FunctionComponent and ComponentClass breaks LibraryManagedAttributes due to the static defaultProps property on those interfaces being set to optional.

import { FunctionComponent, ComponentClass, Component } from 'react'

export interface Props {
  foo: string
  bar?: boolean
}

const TestFunction: FunctionComponent<Props> = props => <div />
TestFunction.defaultProps = {
  foo: '',
}

const TestClass: ComponentClass<Props> = class TestClass extends Component<Props> {
  static defaultProps = {
    foo: '',
  }

  render() {
    return <div />
  }
}

// type is never
type TestDefaultProps = typeof TestFunction extends { defaultProps: infer D } ? D : never
type TestClassDefaultProps = typeof TestClass extends { defaultProps: infer D } ? D : never

// type is Props because typeof Test does not extend { defaultProps } but rather { defaultProps? }
type TestManagedProps = JSX.LibraryManagedAttributes<typeof TestFunction, Props>
type TestClassManagedProps = JSX.LibraryManagedAttributes<typeof TestClass, Props>

This causes defaultProps to be completely ignored by JSX.LibraryManagedAttributes. We should probably remove it as a recommendation from the cheat sheet for now.

@ferdaber
Copy link
Collaborator Author

Some more context on the issue. I even forgot about it... shame on me. DefinitelyTyped/DefinitelyTyped#30695

@swyxio
Copy link
Collaborator

swyxio commented Feb 14, 2019

thanks - i’ll update accordingly. why is it considered a bad idea to have implicitly typed children? i thought that was a major benefit of using React.FC.

as a side note - i dont find myself ever using defaultProps for function components as i can assign when i destructure...

@eps1lon
Copy link
Member

eps1lon commented Feb 14, 2019

as a side note - i dont find myself ever using defaultProps for function components as i can assign when i destructure...

  1. Makes parsing a component easier: You don't have to look through the function body to know what the default props are. Even more so you don't have to care about the specific component implementation. Just scan for defaultProps and ignore whether it is a class, function, forwardRef etc.
  2. tools like react-docgen only understand defaultProps Actually I'm not sure how good react-docgen is in this regard. Does it only know function ({ foo = 'default' }) or can it find the default value reliably?

swyxio added a commit that referenced this issue Feb 14, 2019
update React.FC and defaultProps recommendations with reference to #87
@swyxio
Copy link
Collaborator

swyxio commented Feb 14, 2019

i've put in something quick to reflect this info. It's not clear to me if we should just blanket advise against using React.FC altogether, that feels like a very strong recommendation that we'd have to justify just because of defaultProps. I'm open to it, but simply havent done it yet. Happy to hear your thoughts.

@eps1lon
Copy link
Member

eps1lon commented Feb 14, 2019

It's not clear to me if we should just blanket advise against using React.FC altogether

I wouldn't do that either. I think it's perfectly fine to explain the drawbacks of

explicit annotations on function expressions:

  • -no defaultProps
  • +explicit type annotation (helps with some gotchas concerning return types of function components)
  • -no additional statics
  • -no generic props

function declarations:

  • +defaultProps in ts >= 3.1
  • no implicit children
  • -needs explicit return type to catch wrong return type
  • +additional statics
  • +generic props

Personally I don't like implicit children anyway. Not all of my components handle children. I would hope typescript would recognize some day that <Foo>bar</Foo> is equivalent to <Foo children="bar" /> and report an error on both call sites if the props of Foo don't define a children property.

@ferdaber
Copy link
Collaborator Author

ferdaber commented Feb 14, 2019

If I could rewrite react types implicit children would be removed. It causes a lot more problems and was only placed there for convenience.

I always explicitly include children in my props interfaces anyway when I do support them.

@swyxio
Copy link
Collaborator

swyxio commented Feb 14, 2019

cool cool i'll incorporate more or less what @eps1lon suggested. makes sense

@eps1lon
Copy link
Member

eps1lon commented Feb 22, 2019

as a side note - i dont find myself ever using defaultProps for function components as i can assign when i destructure...

@sw-yx Well looks like you're not the only one: Deprecate defaultProps on function components

@Grsmto
Copy link

Grsmto commented May 2, 2019

That's great to know that React actually think about deprecating defaultProps. Could we come up with a pattern that works and does not use defaultProps?

@swyxio
Copy link
Collaborator

swyxio commented May 2, 2019

i mean.. destructure and assign defaults?

const TestFunction: FunctionComponent<Props> = { foo = "bar" } => <div>{foo}</div>

@Grsmto
Copy link

Grsmto commented May 2, 2019

I was thinking more of something along these lines:

const defaultProps = {
  foo: 'bar',
};

type Props = typeof defaultProps & {
  optional?: string
};

export const TestFunction: FC<Props> = props => {
  const { foo } = {
    ...defaultProps,
    ...props,
  };
  return (
    <div>{foo}</div>
  );
};

As that's the pattern that is currently advised in the doc.

@ferdaber
Copy link
Collaborator Author

ferdaber commented May 2, 2019

Default values inside parameters (and when destructured) are better understood by the compiler. It will allow you to not have to hack together the props interface. The compiler knows it will always be defined inside the implementation but optional when consumed.

@Grsmto
Copy link

Grsmto commented May 2, 2019

Right.
You would also have to define your type interface props as optional for all the ones that have a default value. Since you're not using defaultProps anymore.

@slikts
Copy link

slikts commented May 2, 2019

{ foo = "bar" } => <div>{foo}</div> isn't valid syntax, ({ foo = "bar" }) is, and it would also require = {} at the end or else the caller would need to provide an empty object to use the defaults. So:

const TestFunction: FunctionComponent<Props> = ({ foo = "bar" } = {}) => <div>{foo}</div>

Also, there isn't really much reason to use FC with TS and function components, as .defaultProps, .propTypes and .displayName have better alternatives, and .contextTypes is a legacy feature, so, given the nullable .children typing issue FC has, it probably should just be ({ foo = "bar" }: Props = {}).

@swyxio
Copy link
Collaborator

swyxio commented May 3, 2019

yep y'all know what i mean

@swyxio
Copy link
Collaborator

swyxio commented Mar 25, 2020

going to close this now since it seems we havent had much complaints on this front after Seb's initial recommendations

@StudioSpindle
Copy link

StudioSpindle commented Jul 31, 2020

@slikts

"...as .defaultProps, .propTypes and .displayName have better alternatives,..."

I'm having trouble finding the alternative to default props for a functional component.

In the Readme the link to Martin Hochels article mentions you can use type inference for a class component. But that does not work for an FC component. In an earlier article by the same author (from 2018) he mentions a custom getProps function. That covers all the use-cases as explained in the article, but it seems a bit perculiar.

Using prop destructuring gives a linting error (require-default-props). Should this be ignored?

Is that the way to go? An example would help.

@slikts
Copy link

slikts commented Jul 31, 2020

The linter error should be ignored or turned off, because destructuring and default values already achieves the same as .defaultProps. The linter errors are just rules of thumb that are contextual, and this specific rule doesn't apply in this case.

@darkowic
Copy link

We found a little bit of casting here to be a good compromise. Obviously, if you don't need defaultProps, the best is to use what Sebastian has suggested (we need this to generate docs).

import React from 'react';
import PropTypes from 'prop-types';

export type MyComponentProps = {
    optionalButDefaulted?: string;
};

const defaultProps = {
    optionalButDefaulted: 'optionalButDefaulted',
};

export const MyComponent = (props: MyComponentProps) => {
    const { optionalButDefaulted } = props as MyComponentProps &
        typeof defaultProps;

    return <div>The string length: {optionalButDefaulted.length}</div>;
};

export const myComponentPropTypes: React.WeakValidationMap<MyComponentProps> = {
    optionalButDefaulted: PropTypes.string,
};

MyComponent.propTypes = myComponentPropTypes;
MyComponent.defaultProps = defaultProps;
MyComponent.displayName = 'MyComponent';

bernssolg added a commit to bernssolg/My-React-Sample that referenced this issue Feb 28, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
erinodev added a commit to erinodev/My-React-project that referenced this issue Feb 28, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
petardev101 added a commit to petardev101/react that referenced this issue Jun 4, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
supercrytoking added a commit to supercrytoking/react that referenced this issue Jul 14, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
kevindavies8 added a commit to kevindavies8/react-full-stack-developer that referenced this issue Aug 24, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
johnfrench3 pushed a commit to johnfrench3/react-Fronted-developer that referenced this issue Sep 7, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
ericbrown2716 added a commit to ericbrown2716/react-stack-build-website that referenced this issue Sep 29, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
peterjohnson4987 added a commit to peterjohnson4987/full-stack-developer-react that referenced this issue Oct 3, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
renawolford6 pushed a commit to renawolford6/react-husky-website that referenced this issue Oct 6, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
Yoshidayoshi23 added a commit to Yoshidayoshi23/react that referenced this issue Oct 20, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
renawolford6 added a commit to renawolford6/react-dev-build-doc- that referenced this issue Nov 10, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
coopfeathy added a commit to coopfeathy/cheatsheet that referenced this issue Dec 4, 2022
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
dreamcoder75 added a commit to dreamcoder75/react-sample that referenced this issue Jan 15, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
blackphantom0221 added a commit to blackphantom0221/typescript-cheatsheets-react that referenced this issue Jan 23, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
tamatashi pushed a commit to tamatashi/chart that referenced this issue Feb 27, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
spartacus0816 pushed a commit to spartacus0816/react_frontend_master that referenced this issue May 20, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
AIDevMonster added a commit to AIDevMonster/Awesome-React that referenced this issue Jun 21, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
whiteghostDev added a commit to whiteghostDev/Awesome-React that referenced this issue Aug 6, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
cedev935 added a commit to cedev935/React-TypeScript that referenced this issue Sep 11, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
aleksandaralek added a commit to aleksandaralek/typescript-react-cheatsheet that referenced this issue Oct 24, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
xbucks added a commit to xbucks/react-cheatsheets that referenced this issue Oct 24, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
joyfulmagician added a commit to joyfulmagician/react that referenced this issue Oct 25, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
le-pus pushed a commit to le-pus/React-Typescript that referenced this issue Oct 26, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
atsumi000105 added a commit to atsumi000105/typescript-cheatsheets-react that referenced this issue Dec 8, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
secretsuperstar1109 added a commit to secretsuperstar1109/react-typescript-cheatsheets that referenced this issue Dec 9, 2023
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
champion119 added a commit to champion119/react that referenced this issue Jan 5, 2024
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
SManOlaf38 pushed a commit to SManOlaf38/react_typescript that referenced this issue Feb 20, 2024
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
SManOlaf38 added a commit to SManOlaf38/react_typescript that referenced this issue Feb 20, 2024
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
rising-dragon360 added a commit to rising-dragon360/react that referenced this issue Mar 13, 2024
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
EugeneYoona added a commit to EugeneYoona/React_full_src that referenced this issue Apr 10, 2024
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
snowMan128 added a commit to snowMan128/TypeReact that referenced this issue Apr 15, 2024
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
fairskyDev0201 added a commit to fairskyDev0201/typescript-cheatsheet that referenced this issue Apr 17, 2024
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
bluesky4293 added a commit to bluesky4293/React that referenced this issue Apr 27, 2024
update React.FC and defaultProps recommendations with reference to typescript-cheatsheets/react#87
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

7 participants