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

Missing prop validation in React.FunctionComponent #2353

Closed
dchebakov opened this issue Jul 18, 2019 · 83 comments
Closed

Missing prop validation in React.FunctionComponent #2353

dchebakov opened this issue Jul 18, 2019 · 83 comments

Comments

@dchebakov
Copy link

Tell us about your environment

  • ESLint Version: v6.0.1
  • Node Version: v10.16.0
  • npm Version: v6.9.0

Configuration

module.exports = {
  parser: '@typescript-eslint/parser',
  parserOptions: {
    project: './tsconfig.json',
  },
  plugins: ['@typescript-eslint'],
  extends: [
    'plugin:react/recommended',
    'plugin:@typescript-eslint/recommended',
  ],
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

import * as React from 'react';
interface PersonProps {
    username: string;
}
const Person: React.FunctionComponent<PersonProps> = (props): React.ReactElement => (
    <div>{props.username}</div>
);
npx eslint src --ext .tsx

What did you expect to happen?
eslint show no errors, because type definition actually present in generic type React.FunctionComponent<PersonProps>

What actually happened? Please include the actual, raw output from ESLint.

error    'username' is missing in props validation       react/prop-types
@ljharb
Copy link
Member

ljharb commented Jul 18, 2019

Technically you typed the variable and not the function; what happens if you use a normal non-arrow function for your component?

@dchebakov
Copy link
Author

Technically you typed the variable and not the function; what happens if you use a normal non-arrow function for your component?

For non-arror function component:

function Person(props: PersonProps): React.ReactElement {
    return (
        <div>{props.username}</div>
    )
}

there are no errors, but I want to use arrow func with generic type FunctionComponent. For that case, type of props should be inferred from React.FunctionComponent<PersonProps>

@ljharb
Copy link
Member

ljharb commented Jul 18, 2019

That’s something typescript does, but unless the typescript eslint parser passes those types to this plugin, we have no way of leveraging them.

It’s a better practice to use non arrows for SFCs anyways, so they have a proper reliable name for debugging.

@dchebakov
Copy link
Author

Should I create an issue for typescript parser?

@ljharb
Copy link
Member

ljharb commented Jul 19, 2019

I’m not sure. You could certainly try a PR here to improve the type detection - before filing an issue even on the typescript parser you’d need to have tested and figured out what was needed.

The simplest fix, though, is to follow what the most popular styleguide recommends and use normal functions for components.

@tx-steven
Copy link

I'm having this same error but with functional components. Can't seem to find any good reason why it's happening. I have a functional component. With a typescript props interface. When I access a property off of the prop. I get <property> is missing in types validation.

@ljharb
Copy link
Member

ljharb commented Sep 21, 2019

@tx-steven is it an arrow function or a normal function?

@skube
Copy link

skube commented Sep 24, 2019

Adding : PersonProps (somewhat redundantly) seems to work...

const Person: React.FunctionComponent<PersonProps> = (props: PersonProps): React.ReactElement => (
    <div>{props.username}</div>
);

@ljharb
Copy link
Member

ljharb commented Sep 25, 2019

That’s not redundant; in your case the first one is typing the variable, not the function.

@skube
Copy link

skube commented Sep 25, 2019

I guess but I thought TypeScript infers the types — suggesting that it's not required to do both left and right operands.

@shulcsm
Copy link

shulcsm commented Sep 25, 2019

It does, this rule is redundant for arrowfunc: React.FC<> case since props are checked by TS

@ljharb
Copy link
Member

ljharb commented Sep 25, 2019

TypeScript may, but if the eslint typescript parser doesn’t, there’s nothing this plugin can do.

@Rey-Wang
Copy link

@skube it works

@aaronlna
Copy link

aaronlna commented Dec 2, 2019

@skube A small formality, but the inferred type is more specifically React.PropsWithChildren<PersonProps>. Although if you aren't using any of its (PropsWithChildren) attributes, PersonProps would be sufficient.

If this isn't an eslint issue would creating an option to ignore this case be worthwhile?

@kristojorg
Copy link

This is quite annoying, and I don't believe there is any value in using non arrow functions for function components.

@ljharb
Copy link
Member

ljharb commented Dec 5, 2019

it has lots of value; explicitly that you aren’t relying on name inference, which is unintuitive.

@mohsinulhaq
Copy link
Contributor

mohsinulhaq commented Jan 18, 2020

It’s a better practice to use non arrows for SFCs anyways, so they have a proper reliable name for debugging.

One advantage of using arrow function is the children typing you get for free. For example:

interface CompProps {
    prop1: number
}

const Comp: React.FC<CompProps> = ({ children, prop1 }) => {...} // children inferred from React.FC

function Comp({ children, prop1 }: CompProps) {...} // where to put React.FC?

ref: TypeScript#16918

It works in TSLint but not in ESLint.

@skube
Copy link

skube commented Jan 18, 2020

function Comp({ prop1 }: CompProps) {...} // where to put React.FC?

Can't you do? :

function Comp({ prop1 }: CompProps): React.ReactElement {...} 

@mohsinulhaq
Copy link
Contributor

function Comp({ prop1 }: CompProps) {...} // where to put React.FC?

Can't you do? :

function Comp({ prop1 }: CompProps): React.ReactElement {...} 

Sorry I missed destructuring children in the second component. Added it now. After that, you face the same problem.

@xjamundx
Copy link
Contributor

xjamundx commented Jan 24, 2020

I found this to be an interesting case

// fails with prop-types
export const BlockBad: React.FC<BlockProps> = ({ attributeName }) => {
  return <div data-attributeName={attributeName} />;
};

// does not fail with prop-types
export const BlockGood: React.FC<BlockProps> = ({ attributeName }) => {
  if (attributeName) {
    return <div data-attributeName={attributeName} />;
  }
};

One workaround is to double-type your components, but it's definitely not behaving consistently as-is

export const BlockBad: React.FC<BlockProps> = ({ attributeName }: BlockProps) => {

For those seeking ideas about how to type these things. I found both of these resources helpful:

Basically one suggests to use function and the other suggests the approach being used by a lot of people in this thread const val: React.FC = () => {}

@leonardorb
Copy link

@xjamundx very nice. This will work without warnings with the latest versions:

import React from 'react'

type CellProps = {
  value: string
}

const Cell: React.FunctionComponent<CellProps> = ({ value }: CellProps) => {
  return <div className="item">{value}</div>
}

export default Cell

@saranshkataria
Copy link

saranshkataria commented May 9, 2020

@leonardorb that will work, but as @xjamundx pointed out, you are typing it twice. It should have been inferred automatically instead.

Another potential way of doing it would be:

const Cell = ({ value }: CellProps): JSX.Element => {
  return <div className="item">{value}</div>
}

But not sure what the ideal statement should be.

@AlaaZorkane
Copy link

AlaaZorkane commented May 26, 2020

Any updates on this? Or at least a best practice, I don't see how typing twice is acceptable in this case...

@saranshkataria
Copy link

I have manually turned off the react/prop-types rule in my config for now. Not sure if there is a better way to handle it.

@tx-steven
Copy link

Best practice is to just not use arrow functions so I don't think you guys are going to find an easy/simple work around for this if you want to use arrow functions for react components

@TeoTN
Copy link

TeoTN commented Jun 11, 2020

I think the rule is pretty useless in the current shape - it's supposed to check if props are typed, not to enforce whether the project should use arrow functions or not. It's not in scope of its responsibility.

@ljharb
Copy link
Member

ljharb commented Jun 12, 2020

@TeoTN unfortunately by using arrow syntax it seems TS eslint parsers are incapable of attaching the type to an arrow function, so here we are :-/ happy to review a PR that can improve the situation.

@TeoTN
Copy link

TeoTN commented Jun 15, 2020

I don't have a solution at hand. If it's unfeasible at the moment, it should be probably removed from the default set of rules.

@rsimp
Copy link

rsimp commented Aug 17, 2021

My two cents:

  1. While you can make a rule for enforcing arrow functions, realize they are necessary for React.memo, React.forwardRef, or any other type of composition function. Also while they are better for automatically setting display name, any project that uses babel will still have displayName on arrow functions by simply using the preset for react. You can also handle setting displayName through a babel plugin macro. IMO this should never be part of the recommended ruleset, it's too dependent on the toolchain to decide whether it's a good policy or not. Considering React.memo forces you to use variable assignment (blowing away the main reason here for using function) there's even an argument for always using fat arrows for consistency.
  2. React.FC was removed from the create-react-app typescript template because of how it handles children. While a convenient shorthand, you can no longer make children required or disallowed, or restrict it further than ReactNode which is pretty broad. If you're fine with the looser typings on children it's still pretty convenient. I don't think it's relevant to this discussion but I think the community is moving away from it.
  3. You can always turn off this rule and just use explicit module boundary types from typescript-eslint

@ljharb
Copy link
Member

ljharb commented Aug 17, 2021

@rsimp Arrow functions are in no way necessary for any kind of component function, including memo and forwardRef, and arrow functions are WORSE for setting the display name, and the react preset won’t always ensure it’s present. React.memo takes a function as an argument, so variable assignment can’t possibly infer the display name.

@rsimp
Copy link

rsimp commented Aug 17, 2021

@rsimp Arrow functions are in no way necessary for any kind of component function, including memo and forwardRef, and arrow functions are WORSE for setting the display name, and the react preset won’t always ensure it’s present. React.memo takes a function as an argument, so variable assignment can’t possibly infer the display name.

I may have the wrong babel preset listed but here's a quote from gaearon in a react-devtools issue

If you use Babel, es2015 preset contains function-name plugin that infers the name based on left hand side in expressions.

For Facebook internal use case, @spicyj recently fixed it by enabling the same transform.

As for React.memo I believe only in React 17 and only since late 2020/early 2021 can you get a nice displayName with a named function without setting it explicitly. If eslint-plugin-react wanted to enforce named functions you'd want to include inlined named functions inside of React.memo too. Also unless you used a default export you'd be naming the component twice in your code. Once for the named function, and again for the React.memo variable assignment. Or you could use a babel macro to prevent the duplication and set it for you.

My main point is that tooling can be setup so that a component created with a fat arrow function can have a display name without you explicitly setting it. If someone (and I don't think it's that uncommon) has this kind of project setup, then does it make sense to put an "enforce function components" rule into the recommended ruleset? I don't think it's a bad idea for a rule, but you don't want a rule to be "recommended" if it'll collide with projects on something this arbitrary. All that should matter is that displayName is set. eslint-plugin-react shouldn't be enforcing how it's done. If there's not enough information to do it automatically then you shouldn't be setting the rule automatically either

@rsimp
Copy link

rsimp commented Aug 17, 2021

@ljharb you're right though, fat arrow is definitely not necessary for React.memo React.forwardRef, thanks for the correction. But I think the main point still stands. In a lot of cases with function composition or component factories, display name resolution often gets tricky. Simply enforcing named functions isn't enough. Every library would have to conditionally utilize the passed in function's name (if set). Otherwise you still need to set it yourself or use a code mod like styled components does.

It also gets pretty verbose using more than one composition function together. Let's say React.memo and React.forwardRef. I'm sure people will want to handle these types of cases in different ways. I'm not sure it makes for a good "recommended" rule. Just my opinion.

@ljharb
Copy link
Member

ljharb commented Aug 17, 2021

For React, named functions are absolutely enough. If you can find a popular tool that doesn't respect a component's name when available, call it out and i'll send them a PR fixing it.

Dan himself says that components shouldn't be arrow functions, because of the name in particular. It is a perfect candidate for being a recommended rule - components should be regular functions.

@Chaoste
Copy link

Chaoste commented Dec 17, 2021

I would like to mention another case where I couldn't resolve the linting errors after testing all the proposed solutions in this thread (turning off this lint rule should not be the solution but rather a temporary workaround):

import React from 'react';

type MySvgIconProps = React.SVGProps<SVGSVGElement>;

export function MySvgIcon(props: MySvgIconProps) {
  const withDefaults = {
    ...props,
    width: props.width ?? 100,
    height: props.height ?? 100,
    color: props.color ?? '#fff',
  };

  return (
    <svg
      xmlns="http://www.w3.org/2000/svg"
      enableBackground="new 0 0 24 24"
      viewBox="0 0 24 24"
      {...withDefaults}>
      <rect fill="none" height="24" width="24" />
      <path
        d="M1,1C20,20,16,-8"
        fill={withDefaults.color}
      />
    </svg>
  );
}

Error:

   9:18  error  'width' is missing in props validation   react/prop-types
  10:19  error  'height' is missing in props validation  react/prop-types
  11:18  error  'color' is missing in props validation   react/prop-types

Modules:

"@typescript-eslint/eslint-plugin": "3.10.1",
"@typescript-eslint/parser": "3.10.1",
"eslint": "7.26.0",
"@types/react": "16.14.6",
"react": "16.14.0",

While typescript understands it fully and doesn't complain, eslint probably doesn't understand React.SVGProps<SVGSVGElement> fully (which defines the used properties)

@colemars
Copy link

This rule is just doubling up on the job TS is already doing. Don't recommend.

@ljharb
Copy link
Member

ljharb commented Jan 29, 2022

@colemars no, it's not, because it can check runtime values, and subsets like integers, and all sorts of things TS is capable of doing. In addition, TS has the flaw that it doesn't properly differentiate the external types of your props, with the internal type of your props, so you can run into situations where your types lie to you.

Always use propTypes, especially when using TS.

@soujvnunes
Copy link

I think I'm late here, although if you are using ESlint with TypeScript, be sure to extend "plugin:@typescript-eslint/recommended", in the .eslintrc project file.

samyakb added a commit to BuildAreaOrg/buildarea-ui that referenced this issue Apr 20, 2022
For now this rule is turned offed. I looked into [this
issue](jsx-eslint/eslint-plugin-react#2353).
But didn't understand much
@AdrianDiazG
Copy link

why is this thread closed? the way this rule works right now seems obviously redundant if you wanna use arrow functions and React.FC, and no, changing to the other function definitions doesn't look like a good solution, it's just ignoring the problem

@ljharb
Copy link
Member

ljharb commented Oct 3, 2022

@AdrianDiazG you're not supposed to do that tho - components should be named functions.

@jiminikiz

This comment was marked as off-topic.

@ljharb

This comment was marked as off-topic.

@jiminikiz

This comment was marked as off-topic.

@ljharb

This comment was marked as off-topic.

@jiminikiz

This comment was marked as off-topic.

@ljharb

This comment was marked as off-topic.

@jiminikiz

This comment was marked as abuse.

@ljharb

This comment was marked as off-topic.

@jiminikiz

This comment was marked as off-topic.

@linonetwo
Copy link

linonetwo commented Oct 31, 2022

One use case is I expose export type IRenderer = FC<IRendererProps>; in my lib,

And on user side, he uses export const Pawn: IRenderer = (props) => { //...  and get this error.

Is it really necessary to ask user do export const Pawn : IRenderer = (props: IRendererProps) => { on his side?

@dbwcooper
Copy link

dbwcooper commented Jan 11, 2023

I fixed the error

'name' is missing in props validationeslint[react/prop-types](https://github.com/jsx-eslint/eslint-plugin-react/tree/master/docs/rules/prop-types.md)
import { useEffect } from 'react';

interface Props {
  name: string;
}

const App: React.FC<Props> = ({ name }) => {
  useEffect(() => {}, []);
  return <div>{name}</div>;
};

export default App;

error disappears after importing React, although I still don't understand why it worked

import React, { useEffect } from 'react';

interface Props {
  name: string;
}

const App: React.FC<Props> = ({ name }) => {
  useEffect(() => {}, []);
  return <div>{name}</div>;
};

export default App;

@ljharb
Copy link
Member

ljharb commented Jan 11, 2023

Because React.FC doesn’t make sense if React isn’t in scope.

@ericbenedetto16
Copy link

Because React.FC doesn’t make sense if React isn’t in scope.

@ljharb Now that an explicit React import is not required to use JSX, that shouldn't be the case for folks on React 17+ ? Needing to have import React from 'react' or import type { FC } from 'react' is a poor experience especially when the type definitions point to the same file.

@ljharb
Copy link
Member

ljharb commented Mar 24, 2024

@ericbenedetto16 with that transform - which can be used earlier than react 17, and doesn't have to be used after - React doesn't need to be in scope for jsx syntax, is all. React isn't magically in-scope unless you import it in.

The quality of that experience is irrelevant; it's just how JavaScript works.

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

No branches or pull requests