Skip to content

Update react config to whitelist idomatic react patterns #22

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

Open
danielnixon opened this issue Feb 28, 2023 · 4 comments
Open

Update react config to whitelist idomatic react patterns #22

danielnixon opened this issue Feb 28, 2023 · 4 comments

Comments

@danielnixon
Copy link
Contributor

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
Copy link
Collaborator

Tim-W-James commented Feb 28, 2023

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
Copy link
Contributor Author

danielnixon commented Feb 28, 2023

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
Copy link
Contributor Author

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

  • functional/functional-parameters
  • functional/no-expression-statements

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants