[eslint-plugin-react-hooks] Disallow hooks in class components #18341
Conversation
Per discussion at Facebook, we think hooks have reached a tipping point where it is more valuable to lint against potential hooks in classes than to worry about false positives. Test plan: ``` # run from repo root yarn test --watch RuleOfHooks ```
There was a problem hiding this comment.
Looks OK. One interesting aspect is this would flag non-React classes that use Hooks inside too. That may be a bad pattern anyway, but some people might be upset. I wonder if it's worth limiting it to React.(Pure?)Component subclasses and/or classes with a render method.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7627a7d:
|
Details of bundled changes.Comparing: 6b7281e...7627a7d eslint-plugin-react-hooks
Size changes (stable) |
Details of bundled changes.Comparing: 6b7281e...7627a7d eslint-plugin-react-hooks
Size changes (experimental) |
Running this on our internal codebase at Facebook, I caught a bug in some code that was using a hook in a non-React class. I think it may be safe to say if you are using this lint rule, you should stick to the convention that all |
|
ok let's do it |
|
We have multiple cases in our codebase which look something like class StatefulStuff {
// ...
// A bunch of stateful functionality that can change over time.
// ...
// A helper hook that can allow a functional component to re-render when state changes.
useTheThing(key: string) {
const [thing, setThing] = useState(this.map[key])
useEffect(() => { /* Register an event handler, etc. */ })
return thing
}
}What can be done to make this ESLint rule stop thinking that this 99% non-React code is a React component? I'm happy to help implement the detection provided some pointers, because unfortunately as it stands right now we're given 2 pretty bad choices for our code base:
If there are other workarounds I'm all ears. |
|
Are hooks allowed in class based components? |
|
I'm running into the same issue as @blixt above. I have a stateful non-React class for API accesses that I pass around the app, and I define a |
|
You can avoid linting errors the following way: // A helper hook that can allow a functional component to re-render when state changes.
const useTheThing = (key: string) => {
const [thing, setThing] = useState(this.map[key])
useEffect(() => { /* Register an event handler, etc. */ })
return thing
}
class StatefulStuff {
// ...
// A bunch of stateful functionality that can change over time.
// ...
// ⚠️ Pesonal note: not sure this is a good idea even thought the code is syntactically correct
useTheThing = useTheThing;
}TBH, I cannot imagine a real use case that would need a hook function inside a non-react class. Implementing things this way may lead to confusion by new contributors to the codebase, needlessly making the learning curve steeper. |
|
@kachkaev Rebinding the function might be a working solution, though as your example uses an arrow function it cannot be rebound and will not work. I think it can be made to work with a regular
I think a regular class does very little to confuse people – it's traditional JavaScript syntax. If the user understands hooks already, I don't think I can also tell you about a real use case that we're actively using across our entire React codebase, which led to my comment in the first place: // (Very simplified to save space.)
export class FeatureSet {
#activeConfig
#listeners = new Map()
constructor(config) {
this.#activeConfig = { ...config }
}
update(changes) {
for (const name in changes) {
const variant = changes[name]
this.#activeConfig[name] = variant
const set = this.#listeners.get(name)
if (!set) continue
set.forEach(listener => listener(variant))
}
}
useCondition(name, condition) {
// This is a pretty involved hook that is hydration aware among other things
// and uses internal state of FeatureSet so it would not be nice to move it out.
}
// more methods for inspecting state, adding listeners, etc
}
// Example use
// In some shared module:
const experiments = new FeatureSet({ checkoutEncouragement: "off" })
// In some app logic somewhere:
experiments.update({ checkoutEncouragement: "on" })
// Component:
function BuyButton() {
const encourageUser = experiments.useCondition("checkoutEncouragement", v => v === "on")
return <button>{encourageUser ? "BUY NOW!" : "Continue"}</button>
}This is pretty much how we use it, overly simplified of course. We have multiple types of feature sets (tracking platform, active features based on user profile, and experiments for testing). The class tracks several internals such as realtime updates to the current state, and also tracks build-time state for effective hydration of the application. This could of course all be rewritten to use some singleton store that has functions accessing it, but it's not the ideal way to write it, and I see little reason to go to such efforts to appease a linter. |
This comment has been minimized.
This comment has been minimized.
Summary
Per discussion at Facebook, we think hooks have reached a tipping point where it is more valuable to lint against potential hooks in classes than to worry about false positives.
Test Plan