agiledigital-labs / eslint-config-agile-digital

Agile Digital's standard ESLint config
MIT License
3 stars 3 forks source link

Update react config to whitelist idomatic react patterns #22

Open danielnixon opened 1 year ago

danielnixon commented 1 year ago

Example: functional/no-expression-statement is too noisy in the face of hooks. That rule does have an ignorePattern option that could be used to whitelist useFoo statements under the assumption that they're hooks. https://github.com/eslint-functional/eslint-plugin-functional/blob/main/docs/rules/no-expression-statements.md#ignorepattern

Tim-W-James commented 1 year ago

Something I've noticed is total-functions/no-unsafe-mutable-readonly-assignment can be problematic with dependency arrays for useEffect, etc. hooks. Using a readonly type here prevents another rule from parsing it properly, since it is no longer an array literal - react-hooks/exhaustive-deps: React Hook useEffect was passed a dependency list that is not an array literal. This means we can't statically verify whether you've passed the correct dependencies.

So we can't satisfy both rules. I'd rather know if something is missing from my dep array here than use a readonly type, since it is being passed into React anyway. Perhaps there is some way this rule can be disabled for hook dependency arrays specifically?

danielnixon commented 1 year ago

That rule (total-functions/no-unsafe-mutable-readonly-assignment) is pretty hardcore and doesn't have a great Pearson metric score (it's written and maintained by one random guy: https://github.com/danielnixon/eslint-plugin-total-functions). It has some limitations (performance especially) that may make it impractical even if one does accept the premise that the linter (or even the language) should complain when a developer assigns a mutable value to a readonly value or vice versa.

I guess that's the long way of saying we should consider disabling it in the default/recommended config. Especially for the react config, but maybe even in the base config too.

danielnixon commented 1 year ago

We've added whitelisting for hooks to two rules so far:

It's based on a clumsy function name heuristic (useFoo), but that's as good as we can do in this context.

I'll leave this issue open because there's certainly more work to do yet.

danielnixon commented 1 year ago

We should probably whitelist React components and JSX elements from prefer-immutable-types too. cc @haolinj