eslint-plugin-react: Missing prop validation in React.FunctionComponent
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
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Reactions: 152
- Comments: 83 (31 by maintainers)
Commits related to this issue
- Try to fix eslint typings React.FC isn't being respected See also: https://github.com/yannickcr/eslint-plugin-react/issues/2353 — committed to tony/cv by tony 4 years ago
- eslint: Ignore react/prop-types See also: https://github.com/yannickcr/eslint-plugin-react/issues/2353 — committed to tony/cv by tony 4 years ago
- chore: update eslint for react/prop-types: off For now this rule is turned offed. I looked into [this issue](https://github.com/jsx-eslint/eslint-plugin-react/issues/2353). But didn't understand much — committed to BuildAreaOrg/buildarea-ui by samyakb 2 years ago
Support or ignore prop-types in mixed JS/TypeScript projects
Adding
: PersonProps(somewhat redundantly) seems to work…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
For non-arror function component:
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>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.
I agree that due to a flaw in the typescript eslint parser, when using typescript and when writing components as arrow functions, you may find this rule annoying. If that’s the case, feel free to turn it off - but the rule is functioning perfectly, the side effect doesn’t affect most people since non-arrow components are a best practice and the majority aren’t using TS, and I encourage you to file an issue on the TS eslint parser if you want it addressed.
Since there’s nothing actionable for this project here until the TS eslint parser is fixed, I’m going to close this, but will be more than happy to reopen if that changes.
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.
One advantage of using arrow function is the
childrentyping you get for free. For example:ref: TypeScript#16918
It works in TSLint but not in ESLint.
I guess but I thought TypeScript infers the types — suggesting that it’s not required to do both left and right operands.
That’s not redundant; in your case the first one is typing the variable, not the function.
Arrow functions don’t hold names by default, but for debugging purposes they can be assigned a
displayNamefield, which enables finding that name in React Profiler, if you need it. “Greppability” is not a great argument either, you can grep for the name of the variable holding it, also any IDE will find usages of arrow function by its variable.My key point here is that
react/prop-typesrule that is by default enabled in the preset, is supposed to check if prop types are provided, not someone is using arrow functions. Enforcing debatable code style is not within the scope of the rule intent, is it? If the rule is not fulfilling its purpose, because it doesn’t work with one of the most common ways of writing components, it shouldn’t be a part of recommended linting rules for React. Probably we shouldn’t enforce code style, there’s prettier for that, there should be only genuine issues detected.@xjamundx very nice. This will work without warnings with the latest versions:
Technically you typed the variable and not the function; what happens if you use a normal non-arrow function for your component?
I think you’re consistently missing my point, that the purpose of this rule is to check props - it is not
react/enforce-function-keyword. It’s cool that you want to always write the lengthy way and you like it but its not the only way to ensure you have debug names, and there’s no reason to push your solution. But the key point here is that the rule is not serving its purpose well not that it has an unrelated side-effect that is opinionated and probably unneeded.Any updates on this? Or at least a best practice, I don’t see how typing twice is acceptable in this case…
I found this to be an interesting case
One workaround is to double-type your components, but it’s definitely not behaving consistently as-is
For those seeking ideas about how to type these things. I found both of these resources helpful:
Basically one suggests to use
functionand the other suggests the approach being used by a lot of people in this threadconst val: React.FC = () => {}It does, this rule is redundant for arrowfunc: React.FC<> case since props are checked by TS
@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:
But not sure what the ideal statement should be.
I just stopped tracking this issue : by adding
"rules": { "react/prop-types": "off" }in .eslintrc.jsThe bundle gets gzipped; you save nothing consequential, and that is very much less important than debuggability anyways.
@ljharb Why do you say components shouldn’t be arrow functions? I haven’t found the reasons yet (not saying they don’t exist).
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.
Can’t you do? :
This rule is just doubling up on the job TS is already doing. Don’t recommend.
@ITenthusiasm i’d prefer a new issue be filed that clearly outlines the problem and the needed solution - this thread is quite long and full of distractions.
Yes, there ideally should be an eslint rule forbidding functional components to be written as arrow functions; i’m not sure why we don’t have one yet.
I know this is closed until new changes, just want to drop here an info about something I didn’t read in above comments.
Same for this alternative
@AdrianDiazG you’re not supposed to do that tho - components should be named functions.
Returning to this conversation again after quite some time to make 2 comments. The first one is just to get people thinking. You don’t have to care about it. 🙂
First, although I mentioned earlier that I typically use regular functions, I have found
React.FCappealing in the past because it created a simple way to make components in React with TypeScript. The place I’m working at now usesReact.FCfrequently. However, after trying to make some re-usable and non-simple components, I realized something and wanted to give a warning:I think we’re used to
React.FCbecause it’s what we’re given out of the box. But actually,React.FCmakes your components less specific than they should be. Here are 2 basic examples. There are likely more.1) Children
From gearFifth’s comment (mohsinulhaq’s comment had something similar):
At first this made me sad. But I’ve learned this is actually a really good thing. Although in reality all React components can take a
childrenprop, not all components are intended to take achildrenprop. Similarly, any regular function you define can take more than the specified number of arguments. However, that’s not the intent of the function and it won’t accomplish anything. Supplying extraneous arguments will only cause confusion. This is part of the point of TS to begin with.“But I actually intend to use the
childrenprop for such and such component”. That’s a valid argument. But in that case, it’s a much better developer experience to explicitly supplychildrenin yourComponentPropstype/interface for two reasons:childrenprop.childrenprop, preventing any confusion.On that second note, let’s say that you have a
Cardcomponent that’s intended to take text as its children. And it only expects text. The defaultchildrenprop fromReact.FCis aReactNode. This means TS will not care if your developer passes some jank JSX into the component and breaks it because you didn’t tell them what was expected by your types. Thus, it’s friendlier to explicitly type yourchildrenanyway.Unfortunately,
React.FCalso has the potential to get in the way of your explicitly definedchildrenprop. For instance, when I try to say “Hey, use astringchild”,React.FCsays, “Oh. You wantedstring & ReactNodeforchildren”. In the end, TS seems to restrict it properly. But that can still potentially be a source of confusion.2) Return Types
This is the thing that really bothers me, to be honest. I absolutely hate specifying return types on components. And yet… I’ve come to see they’re useful while making some re-usable or non-simple components. The default return type from
React.FCis nice, but it’s also not always valid.React.FCleaves the door open for a component to returnnull, but maybe you have components that are never intended to returnnull. Again, stricter, more explicit type guards protect you from this issue. They also prevent TypeScript from complaining about how a component could benullwhen you know you’ve written your code so that it never would benull.Regarding this
defaultPropsandpropTypesare taken care of by TS.contextTypesand thecontextargument are legacy from what I understand and are taken care of by useContext. And an explicitchildrenprop and return type provide a better developer experience with clearer type specificity (…long term, that is… I get the pain of not liking to write it out).Of course, you may not always care about this level of specificity. But oftentimes you do, and that’s worth nothing. (And if you don’t care, it might be worth asking why you’re interested in TS.) Defaulting to regular functions instead of
React.FCs increasingly seems like more of a good idea to me as I learn more (except for the fact that I have to keep specifying return types 😔).The problem with the default
FCtype provided by React is that it tries to be JavaScript and TypeScript at the same time. That is, it acknowledges the fact that the JS functionality will allow you to do whatever you want (like adding extraneous args). And it seems to try to keep the type “backwards compatible” for anyone working with legacy code. So it creates a TS type to cover all of the use cases. But again, this defeats the point of the safety TS aims to provide to begin with. Instead, they should’ve provided the bare minimum types for a functional component.Stated differently (and perhaps far too strongly),
React.FCas it is now – in some sense – is the React version of usinganyin TS… but for functional components.Consider these things when you write your own components…and in the significance of this overall discussion.
TypeScript may, but if the eslint typescript parser doesn’t, there’s nothing this plugin can do.
Because React.FC doesn’t make sense if React isn’t in scope.
with this you lose the default props that come with the functional component like the children prop.
You’re right about
displayName- you can alsoObject.definePropertyanameon it just fine, but the point is that it’s extra effort most people fail to do.It’s supposed to ensure types are provided. It works fine on propTypes for arrow functions - it’s just a failing in the TS parser that it can’t properly type arrow functions.
Use propTypes, and you can use arrow functions all you want. The rule is working perfectly.
Separately, I don’t use prettier because I don’t like some of its style choices, and either way, eslint rules should always have style opinions - prettier users can disable the style rules if they like. Style problems are genuine issues.
@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,PersonPropswould be sufficient.If this isn’t an eslint issue would creating an option to ignore this case be worthwhile?
I fixed the error
error disappears after importing
React, although I still don’t understand why it workedI 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):
Error:
Modules:
While typescript understands it fully and doesn’t complain, eslint probably doesn’t understand
React.SVGProps<SVGSVGElement>fully (which defines the used properties)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.
My two cents:
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 havedisplayNameon arrow functions by simply using the preset for react. You can also handle settingdisplayNamethrough 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. ConsideringReact.memoforces you to use variable assignment (blowing away the main reason here for usingfunction) there’s even an argument for always using fat arrows for consistency.children. While a convenient shorthand, you can no longer makechildrenrequired or disallowed, or restrict it further thanReactNodewhich is pretty broad. If you’re fine with the looser typings onchildrenit’s still pretty convenient. I don’t think it’s relevant to this discussion but I think the community is moving away from it.Second, despite everything that I said in that first comment of mine above, I think there is still an issue that needs to be addressed.
We have an ESLint rule that can’t be disabled by default because it provides good common standards to follow. (There are other React developers that don’t use TS. 😏) At the same time, the linting does seem to be incorrect here for TS. In a theoretical world where
React.FCwas improved to have the bear minimum types, I’m assuming that we’d still run into this issue. So if possible, this problem warrants fixing.The comments about turning this rule off locally are becoming redundant, and they aren’t providing a true solution to the problem. At the same time, comments about only using non-arrow functions still leave the problem at hand. If it’s truly important, restricting components to regular functions would need to be its own ESLint rule…though idk how it would be written. But that shouldn’t bleed into this discussion.
Although I haven’t been able to do anything with it myself yet, the discussion at typescript-eslint/typescript-eslint#3075 provides guidance towards a solution to this problem if I understood it correctly. Since this issue is important, if it is actionable, then @ljharb could you reopen this issue? If it’s nothing that’s being immediately prioritized, maybe something like a
help wantedtag would be fine. But an open issue would more clearly specify intent here.with this you lose the default props that come with the functional component like the children prop.
Oh, I just realize it’s missing one piece of explanation, sorry for that. According to React:
all FunctionalComponents must have a
childrenprop when using typing likeconst t: React.FC<T>.But hey, I’m not disagreeing with you, just here to add info to the conversation.
You should have to have
childrenin your propTypes / props types anyways, it’s a prop like any other.@abmantis yes, but function name inference is implicit, doesn’t work in every browser engine you might be debugging in (or collecting stack traces from), and I’m saying that for clarity/explicitness, it should never be relied upon.
it has lots of value; explicitly that you aren’t relying on name inference, which is unintuitive.
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.