-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
test: Enable react/no-unstable-nested-components
#34518
test: Enable react/no-unstable-nested-components
#34518
Conversation
bfde5b2
to
4e2156a
Compare
@@ -23,7 +23,7 @@ const InputInput = React.forwardRef(function InputInput( | |||
|
|||
const styledInput = <InputUnstyled components={{ Root: InputRoot, Input: InputInput }} />; | |||
|
|||
const PolymorphicComponentTest = () => { | |||
const polymorphicComponentTest = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is that PolymorphicComponentTest
looks like a component (and therefore would allow a hooks call). But inside of it we're declaring new components which is bad. Since it's not actually a component, we adjust the name to reflect that.
const polymorphicComponentTest = () => { | ||
const CustomComponent: React.FC<{ stringProp: string; numberProp: number }> = () => <div />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const polymorphicComponentTest = () => { | |
const CustomComponent: React.FC<{ stringProp: string; numberProp: number }> = () => <div />; | |
const CustomComponent: React.FC<{ stringProp: string; numberProp: number }> = () => <div />; | |
const PolymorphicComponentTest = () => { |
Would this be better? I know that it produces no difference here since we are not using polymorphicComponentTest
but it seems like a component here (or polymorphicComponentTest
is not needed at all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually keep things specific to a test in its scope before hoisting up. That way other tests don't start to rely on it. Up to you, what you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually keep things specific to a test in its scope before hoisting up
That sounds good to me as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could also disable react/no-unstable-nested-components
for the TypeScript tests.
Lines 319 to 320 in 101fa0d
files: ['*.spec.tsx', '*.spec.ts'], | |
rules: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still "invalid React". Better to write it correct everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Left one suggestion.
Enables
react/no-unstable-nested-components
This rule is likely either going to be in the recommended preset or move to rules-of-hooks. Wanted to see how this rule performs on a medium size repo and the errors all look legitimate to me (although it's all just test-code).