-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Non-component functions that return JSX #512
Comments
Actually, stateless functional components can get a second |
I was not aware of this functionality of |
FWIW I'd suggest we stop treating any function that returns JSX as a stateless functional component unless we can also prove that someone is using it in a call to |
It's not just any function that returns JSX - it's functions that return JSX and take a "props" (or destructured) first argument, and an optional "context" second argument. How else would you propose detecting SFCs so that rules can be applied? |
That's not true. I just triggered the The only way you can definitively tell that the user intends to use a function as a SFC is to see if they ever use it as the first arg to |
I think you just have to check if it's anonymous or not. If anonymous, it's not a component. // don't warn
<Blah stuff={({ foo }) => ()}/>
// warn
const Thing = ({ foo }) => () Note, maybe anonymous means something else, another way to phrase this is "is the function we think is a SFC assigned to a variable or not?" |
Yeah, all arrows are anonymous, but I think you're saying, a "maybe SFC" (the current rubric, which as @mjackson points out might not be as smart as I think it should be, including the fact that SFCs don't have to use props at all) that is also exported or assigned to an identifier - as opposed to one that's passed directly as an argument to something that's not I think regardless that the metric for detecting SFCs needs to be as accurate as possible, and it's highly likely that it's both not, and inconsistently applied. However, it's far better imo to have an overly-active lint rule that can be overridden with comments, than to have one that misses otherwise lintable errors. |
What's wrong w/ keeping your current metric and adding "assigned to a variable"? Is there ever a case where you have a SFC that isn't assigned to a variable? |
|
Not all SFCs take props though, right? |
@lencioni yes, you're right - i mentioned that in #512 (comment) |
assigned to variable OR default export |
@ryanflorence if that includes |
Sounds good.
Don't forget that many people like myself will just disable the rule when it's this noisy around false positives (I use these react-motion render props all over the place) and the end effect is no linting for any propTypes, or if they're a beginner be totally confused, so I tend to think the other direction :) |
I'm just worried that other people are going to stumble on this error and think "man, React is hard" just because they made a function that returns some JSX and they're using the "recommended" lint setting. Can we perhaps separate out this functionality into an opt-in sfc-display-name rule? |
I try to reduce false positive/negative when detecting React Components, but it is particularly hard with SFC since they are simple functions and since the linting is made file by file we don't know where/how it will be used. Right now I think we are not too bad at detecting SFC since we have covered most of the common patterns used by the community. Of course there is still some edge cases (like the pattern used here by
Some people have already asked for an
Beside the original case of this issue, did you encountered other problematic patterns with |
I first encountered this error this afternoon while working on the We will have an identical API in the next release of |
Thanks, @yannickcr. I assume you're talking about the work you did in 70bf28d. Just to be clear, how does that work alter the current behavior? |
Yes, the only relevant change here is the addition of The second parameter is the "confidence", when searching for components we rank them with confidence levels:
This is due to the fact that the AST is traversed only one time by ESLint so we need to be sure to store information about components (node, propTypes, etc.) even if we are not quite sure that they are components yet. At the end we only validate the components with a confidence level of 2. With this change if I cannot find a component from an (Hope I was clear, the |
I'll add my vote for an option to disable |
I am getting const options = {
loadingHandler: () => (<LoadingSpinner />),
errorHandler: (error) => (<ErrorBanner error={error} />),
someOtherConfig: [1, 2, 3],
}; Aside from making variables for each, how could I even give this a name? |
By not using arrow functions. const options = {
loadingHandler() { return <LoadingSpinner />; }, // this
errorHandler: function someOtherName() { return <ErrorBanner error={error} />; }, // or this
someOtherConfig: [1, 2, 3],
} |
@ljharb Really? This is clearly not a nice solution.. |
@Kerumen i think it's much nicer. Named functions can be easier to read, because their names convey meaning - and arrow functions don't have explicit names. This is also incredibly important for debugging. |
@zeroasterisk thanks for that, so anonymous functions are not valid as react components? The error for me was because of this:
I had to convert it to:
which fixed the problem. It's weird though, because aComponent is a variable that is assigned an anonymous function... so.... I suppose it considers that as a named function? I don't know, can someone explain this? |
@aprilmintacpineda the function being anonymous or named is irrelevant; the only thing that matters is that the identifier name used in JSX to refer to the element starts with a capital letter. Prior to React 16, any function that returned a node (null, or a React Element) was technically a component - now, a component can also return an array, a string, or a number. |
This really really sucks when you have a file with a rendering function and a component. import React from 'react'
import T from 'prop-types'
const renderItem = ({id, name}) =>
<li key={id}>{name}</li>
const Example = ({items}) =>
<ul>
{items.map(renderItem)}
</ul>
Example.propTypes = {
items: T.arrayOf(T.object).isRequired
}
export default Example The Even worse, there is apparently no way to suppress the inspection on I like the suggestion from @coderitual as a simple solution - as of now the prop-types and display-name inspections, as useful as they could be, are just too painful to enable in my project. |
|
I haven't seen a component which name starts lowercase. Even react is promoting this convention -> https://reactjs.org/docs/components-and-props.html I agree it's not perfect solution but you cannot say whether expression is a SFC or normal function based on expresion itself. What makes the difference is how you call this expression. All custom components in react must start upper case so it's unlikely to see something like this: const comp = () => <div></div>;
const App = () => <comp></comp>; Of course you can export lowercase function and change its name during import but still this is a bad practise. I am not sure why only few people see this relation and simple solution to mitigate the problem. Mike |
I completely disagree. Even if you think that any bit of JSX belongs in a formal component, which seems like a rather arbitrary and onerous requirement, at least a few libraries don't agree with that. For example, I use react virtualized, which employs rendering functions - called from the render methods of other components - to only render what is visible on the screen. Also, extracting a function can greatly improve readability in the case where extracting a component wouldn't add any significant value. edit - typo |
Would the following example be an instance of this issue: function Table(props) {
return <ReactTable columns={[{ /* ... */
accessor: (r) => r.x?"OK":<span className="bad">bad</span>}]}
</ReactTable>
} eslint complains that according to |
yes, because “accessor” is a function that returns jsx. |
I'm getting this too:
What I'm interested in is just how much of an impact this'll have. The rule description says:
but to me that doesn't well enough inform me of the impact that not having a displayName will have. I mean if it's just adding a little bit more detail to debugging messages, then that could be fine completely, provided its not the only piece of information provided for debugging. On the other hand, if it means when an error happens the only thing I'm going to be told is "Component exploded!" without any other tie back to where the error happened, then ensuring every component has a displayName, including very small "middlemen" components like the one in my code above is very important. I know that this can be hard to define, since |
Debugging isn't "just"; it's critical. "when an error happens" is when you need debugging. |
can we have the option to disable the rule when detecting lowercase functions? i'm using this with react-native's SectionList and getting warned for the Assigning propTypes there would do nothing as the check won't be run against functions |
I'd like to be able to exclude lowercase functions as well. Especially with JSX being widespread nowadays, a function name starting with a lowercase character is never a component. |
If you'd rather not disable the rule when creating HOCs, wrapping |
(It shouldn’t; that’s a bug in the component detection) |
I think an option to disable lint errors for a lowercase function is the best way to handle it. |
When functional components were just introduced, I recall writing a lot of “non-component functions that return jsx” mostly as helper functions to a component. However, today, with hooks, I almost never do. Perhaps because components get simpler, or because individual hooks can often be refactored out into the helper functions which makes the latter components in their own right. Although that’s not what OP was about, I feel that today the lint rule heuristic is much less of a nuisance. Overall the rule exists for a reason, so I too am happy annotate the OP as a rare special case. |
None the less, I'd be happy to review a PR that fixed the component detection to treat lowercased-named functions-that-return-jsx as "not components". |
How about not applying prop-types rule to functions inside components? |
Interestingly you cannot mix the way of function definition. E.g. using react-intl:
Gives horrible complains by the TypeScript compiler. |
Is this potentially fixed by #2699 ? |
I believe so; happy to reopen if not. |
Hey @BairDev , were you able to resolve this? |
Hey, not really, I just use the same pattern for the mini-components. |
react-router
allows you to override thecreateElement
function when rendering a route component. For example:This
createElement
function is not a component (a component is a pure function of the props, but in this case it has two parameters, the component and the props), however, eslint recognizes it as one and demands that thepropTypes
be defined. Maybe the component recognition as specified here: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/display-name.md can be narrowed down to functions with only one parameter.Edit: the error I get is
'route' is missing in props validation (react/prop-types)
The text was updated successfully, but these errors were encountered: