fix: modified eslint to support Compound React Components #27900
Conversation
amjadorfali
changed the title
|
Comparing: 49439b4...056f647 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
|
Thanks for working on this :) |
|
We don't really recommend writing components like this because it's bad for code splitting… |
|
Yes, but it's not a violation of the rules of hooks and should therefore not be treated as such. |
|
@tobias-tengler @sophiebits @rickhanlonii @gaearon Apologies, I looked at the github issue labeled with Good First Issue, so I assumed this was something that the maintainers of this project wanted to fix If not, I will close this PR. Thanks! |
|
I think it's ok to support even though the pattern isn't recommended, because if you're going to do this pattern, we should at least lint so you don't have to also turn off the linter for the component. @amjadorfali can you add more test cases? It should fail for things like |
|
@rickhanlonii Done. Can you please review again when possible |
|
Hmm. I feel strongly that we should not merge this. We intentionally decided against cases like this when designing the rule and the heuristics. It blurs the boundaries where it's easy to miss that something is being used as a component. (Extending it to Hooks is especially problematic because people start declaring them as class methods or object methods — and without extending it to Hooks, you'd have an inconsistent approach.) If you like this pattern (which we do not recommend as @sophiebits mentioned), you can always do it like this: const Obj = { Test: {} };
const Test2 = () => {
useEffect(() => {});
};
Obj.Test.Test2 = Test2;That serves as a sufficient escape hatch while keeping component definitions agreeable with the current rule. |
|
@gaearon It's an honor to me that you read my PR, thank you! ❤️ |
|
Haha no worries, I'm sorry for the churn! |
|
To clarify my concern. If you allow this: const Obj = { Test: {} };
Obj.Test.Test2 = () => {
useEffect(() => {});
};Then it's hard to justify why you shouldn't also allow this: const Obj = {
Test: {
Test2: () => {
useEffect(() => {});
}
}
};and this: const Obj = {
Test: {
Test2() {
useEffect(() => {});
}
}
};and this: const Obj = {
Test: {
useTest2() {
useEffect(() => {});
}
}
};or even this: class Obj {
useTest() {
useEffect(() => {});
}
};which is terrible because it's easy to write super broken code this way. It's good that the linter tries to steer you away from it the moment you abandon writing plain functions and try to think of them as "methods" — and that it pushes you back into thinking of them as functions. You can still attach them after the definition if needed. But React has no concept of "compound components" and does not recommend such. (This concept doesn't work with |
|
@gaearon Thanks for the clarification, i understand the issue now. |
|
One approximation of the concept of "compound components" that doesn't rely on attaching functions to each other and that works with all React features is to use the module system. export default function Dropdown() {
// ...
}
export function Item() {
// ...
}import Dropdown, { Item } from './dropdown'
<Dropdown>
<Item />
</Dropdown>Or: export function Box() {
// ...
}
export function Item() {
// ...
}import * as Dropdown from './dropdown'
<Dropdown.Box>
<Dropdown.Item />
</Dropdown.Box>Then you're just using the module system and all features work. |
@gaearon yeah this makes sense to not support, but I think the error message could be improved then, because it doesn't steer you away from this specific pattern. Here's the error: React Hook "useState" is called in function "A.B" that is neither a React function component
nor a custom React Hook function. React component names must start with an uppercase letter.
React Hook names must start with the word "use".I can totally understand that anyone reading this error with this pattern would think it's a bug, because the function satisfies all of the requirement listed. If we want to say that this pattern not a "React function component" then this error could say something like: React Hook "useState" is called in function "A.B". React function components cannot be nested
inside of objects or classes. |
Summary
close #20700
Issue
A React component
fnhaving a property that is a React componentfn, is throwing an eslint error when using Hooks inside that component, due to the naming.Solution
Added support in eslint for node of type
MemberExpression, where they have aproperty.namein capital case, that is aComponentName, andproperty.typeasIdentifier.This also supports Objects, not just React Components:
How did you test this change?