-
-
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
Rule Proposal: functions returning JSX should be refactored into React components #578
Comments
Seems to be an interesting rule. It should be feasible, we already have a isReturningJSX utility function that can already do "jsx fragment functions" detection (but it doesn't cover all cases, like map/reduce). I think it could be used (and improved) for this. I'm definitely interested in this rule. If you can come out with a good solution I'll be happy to merge it :) |
@keithamus wondering if you do anything different almost 2 years later or is this still the best approach for these type of scenarios? |
@eballeste I haven't used react in a while but if I recall, we ultimately resovled to only ever use functional stateless compoments for everything except for components needing the low level life cycle hooks which would naturally get much closer scruitiny. If I was maintaining any react codebases anymore that's the direction I'd certainly move in. |
Two problems:
const addProp = Component => props => <Component prop={'a'} {...props} /> This is not a component but a component wrapper. |
@dirkroorda A component wrapper is itself an SFC component. #1510 is for tracking support of React 16 fragments. |
@ljharb I thought that a component is a function with the signature ({ props }) => R or a class with a render function of that signature, where R is any piece of JSX. The wrapper |
@dirkroorda the wrapper |
The problem with refactoring JSX-returning functions into components is that it can break your tests if you are shallow rendering. For example, in the first case, I could write test case, I might have written a test that asserted that images are rendered for each article. But when I refactored to components, that test would fail, since the |
I’d argue that’s not breaking your tests, it’s improving them - by forcing you to split your tests up so that the new component has its own tests. In other words, shallow rendering means each component primarily has tests that test what it renders, and they delegate testing responsibility for those things to their own tests. |
Hmmm...I see your point in where you refactor out reusable components, but in the case where you just need "helper components" which will only ever be used by one component, they are analogous to private functions and, as such, shouldn't be tested directly. |
For that, enzyme has .dive() |
Now your test is aware of the implementation details of your component. 🙁 Decide that helper component isn't needed, or split it into two, and your tests break again. BTW, enzyme is great, and I love using it. ;) |
Will u explain what are all the rules to be followed for writing jsx,higherorder function,rendermethod |
One potential concern with such a rule: the inline help function for .map inside render returns JSX without being used as a component. Are there any scenarios where similar logic needs to be implemented manually? |
@xsrvmy An argument to map is an anonymous function, so this can distinguish it. The moment it is assigned to either a variable or made into a
This is not a problem because the rule is: IF (return jsx) THEN (make component). If it returns a string, whatever. |
@keithamus @yannickcr This has been open for about 7 years now. Just wondering there is any sort of timeline for this? |
@psyrendust #1580 has some outstanding comments going back 6 years, so no, there's no timeline until someone wants to pick it back up. If you're interested, please comment a link to a rebased branch in that PR, and i'll pull in the changes. |
Hey @yannickcr thanks for being awesome and making this great plugin, super valuable 😎.
I have noticed, that while working with a team of React devs - some devs will write functions that return JSX, but aren't components. This usually makes things difficult to refactor later on, as we need to split out all of these "jsx fragment functions" into real React components.
For example, we often see things like this slip into our codebase:
The problem with the above code, other than from a purity standpoint, is that it becomes a bit of a rats nest at any level of complexity, plus it is difficult to refactor when you need to. Ideally this code would be written as such:
The above code is much easier to reason about - each function that returns JSX has a well defined function signature (that of a react component) and each piece can be easily pulled out into its own component if the time comes.
Proposal
All of the above is just to demonstrate the proposal for the following rule:
The rule would check all functions in a file:
fn(props = {})
)Then the rule would warn that the function should be a React (stateless or class) component.
Let me know what your thoughts are on this. If you're happy with the above proposal I'm happy to put the work into a PR for this.
The text was updated successfully, but these errors were encountered: