Skip to content
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

☂️ eslint-plugin-react-hooks #2174

Closed
Boshen opened this issue Jan 26, 2024 · 15 comments · Fixed by #3315
Closed

☂️ eslint-plugin-react-hooks #2174

Boshen opened this issue Jan 26, 2024 · 15 comments · Fixed by #3315

Comments

@Boshen
Copy link
Member

Boshen commented Jan 26, 2024

Warning

This comment is maintained by CI. Do not edit this comment directly.
To update comment template, see https://github.com/oxc-project/oxc/tree/main/tasks/lint_rules

This is tracking issue for eslint-plugin-react-hooks.

There are 2(+ 0 deprecated) rules.

  • 2/2 recommended rules are remaining as TODO
  • 0/0 not recommended rules are remaining as TODO
    • All done! 🎉

To get started, run the following command:

just new-react-hooks-rule <RULE_NAME>

Then register the rule in crates/oxc_linter/src/rules.rs and also declare_all_lint_rules at the bottom.

Recommended rules

✨: 0, 🚫: 0 / total: 2
Status Name Docs
react-hooks/rules-of-hooks https://reactjs.org/docs/hooks-rules.html
react-hooks/exhaustive-deps facebook/react#14920

✨ = Implemented, 🚫 = Not supported

@rzvxa
Copy link
Contributor

rzvxa commented Apr 20, 2024

@Boshen I wanted to implement these, I've noticed that the command just new-react-hooks-rule <RULE_NAME> doesn't work; I can create the rule manually I just need to know what should I change in order to keep this comment in sync in CI.

@rzvxa
Copy link
Contributor

rzvxa commented Apr 20, 2024

I just noticed that #2637 is a thing, I'm going to only work on the hook rules.

@Boshen
Copy link
Member Author

Boshen commented Apr 20, 2024

I just noticed that #2637 is a thing, I'm going to only work on the hook rules.

#2637 is stale, maybe you can help out and merge the code first, then improve upon it.

@Boshen
Copy link
Member Author

Boshen commented Apr 20, 2024

@Boshen I wanted to implement these, I've noticed that the command just new-react-hooks-rule <RULE_NAME> doesn't work; I can create the rule manually I just need to know what should I change in order to keep this comment in sync in CI.

There are only 2 rules, and the tests cannot be parsed automatically :-/

@rzvxa
Copy link
Contributor

rzvxa commented Apr 20, 2024

@Boshen I wanted to implement these, I've noticed that the command just new-react-hooks-rule <RULE_NAME> doesn't work; I can create the rule manually I just need to know what should I change in order to keep this comment in sync in CI.

There are only 2 rules, and the tests cannot be parsed automatically :-/

Should I put them under react or create a new category? It won't be such a bad thing to unify such categories and not base them strictly on the origin plugin.

@Boshen
Copy link
Member Author

Boshen commented Apr 20, 2024

@Boshen I wanted to implement these, I've noticed that the command just new-react-hooks-rule <RULE_NAME> doesn't work; I can create the rule manually I just need to know what should I change in order to keep this comment in sync in CI.

There are only 2 rules, and the tests cannot be parsed automatically :-/

Should I put them under react or create a new category? It won't be such a bad thing to unify such categories and not base them strictly on the origin plugin.

Yeah, under the react plugin should be fine.

@rzvxa
Copy link
Contributor

rzvxa commented May 13, 2024

✨: 0, 🚫: 0 / total: 2
Status Name Docs
react-hooks/rules-of-hooks https://reactjs.org/docs/hooks-rules.html
react-hooks/exhaustive-deps facebook/react#14920

This didn't update, Have I missed something with the tasks?

PS: please don't just fix it, Let me know what I did wrong so I don't repeat it in the future.

@leaysgur
Copy link
Contributor

leaysgur commented May 13, 2024

That's because react-hooks/* rules are defined under react:: mod, it should be react_hooks:: to be tracked by lint_rules task.

react::rules_of_hooks,

If this is intended, we need to update lint_rules task to mange these conversion.

image

@rzvxa
Copy link
Contributor

rzvxa commented May 13, 2024

That's because react-hooks/* rules are defined under react:: mod, it should be react_hooks:: to be tracked by lint_rules task.

react::rules_of_hooks,

If this is intended, we need to update lint_rules task to mange these conversion.
image

Thanks, Yes it was intentional there are only 2 rules for hooks and I've spoken with @Boshen about merging them into react; And he gave the green light for it.

@Boshen
Copy link
Member Author

Boshen commented May 13, 2024

I'm in favor of modifying the task script.

@rzvxa
Copy link
Contributor

rzvxa commented May 13, 2024

Can you help me with the change?

Should I do it in such a way that this issue gets closed in favor of adding these entries to #1022, Or keep this issue and just resolve the implementation from the react module?

@leaysgur
Copy link
Contributor

FYI: If

keep this issue and just resolve the implementation from the react module?

diff --git a/tasks/lint_rules/src/oxlint-rules.cjs b/tasks/lint_rules/src/oxlint-rules.cjs
index 65302b62..e8ee28c5 100644
--- a/tasks/lint_rules/src/oxlint-rules.cjs
+++ b/tasks/lint_rules/src/oxlint-rules.cjs
@@ -90,7 +90,12 @@ exports.createRuleEntries = (loadedAllRules) => {
 exports.updateImplementedStatus = async (ruleEntries) => {
   const implementedRuleNames = await readAllImplementedRuleNames();

-  for (const name of implementedRuleNames) {
+  for (let name of implementedRuleNames) {
+    // These rules are implemented under `react::` mod, but issue is separated
+    if (name === "react/rules-of-hooks" || name === "react/exhaustive-deps") {
+      name = name.replace("react/", "react-hooks/");
+    }
+
     const rule = ruleEntries.get(name);
     if (rule) rule.isImplemented = true;
     else console.log(`👀 ${name} is implemented but not found in their rules`);

If not, there is a little more work to do. 😅

@rzvxa
Copy link
Contributor

rzvxa commented May 13, 2024

If not, there is a little more work to do. 😅

You seem to have a much clearer idea of this, Do you think it is possible to add react-hook plugins as react here so we can add it to the react issue(in case we even want to)?

exports.createRuleEntries = (loadedAllRules) => {
/** @type {RuleEntries} */
const rulesEntry = new Map();
for (const [name, rule] of loadedAllRules) {
// Default eslint rules are not prefixed
const prefixedName = name.includes("/") ? name : `eslint/${name}`;
const docsUrl = rule.meta?.docs?.url ?? "";
const isDeprecated = rule.meta?.deprecated ?? false;
const isRecommended = rule.meta?.docs?.recommended ?? false;
rulesEntry.set(prefixedName, {
docsUrl,
isDeprecated,
isRecommended,
// Will be updated later
isImplemented: false,
isNotSupported: false,
});
}
return rulesEntry;
};


Edit:

This way we can have something like this in the definition:

  ["react-hooks", { npm: "eslint-plugin-react-hooks", issueNo: 2174, alias: "react" }],

@leaysgur
Copy link
Contributor

(Actually, I am the original author of this task. 🤫 )

I was thinking that there are a few possible ways to handle this.
If it's okay to address it (maybe) this weekend, I can take care of it.

@rzvxa
Copy link
Contributor

rzvxa commented May 13, 2024

(Actually, I am the original author of this task. 🤫 )

Makes sense😅

Let me know if there is anything that I can help with, especially if you didn't have the time to do it.

Boshen pushed a commit that referenced this issue May 16, 2024
Fixes #2174 , now `react-hooks` plugin and its rules are managed by
`react` plugin issue.
Boshen pushed a commit that referenced this issue Nov 12, 2024
Firstly, a massive thanks to @alisnic for starting this (incredibly complicated) lint rule in #2637 !

still a draft. current state:

3x false positives (all todo with refs)

3x false negatives (TS will catch these
13x false negatvies todo with refs
1x false negative TODO

closes  #2637
relates to #2174
Dunqing pushed a commit that referenced this issue Nov 17, 2024
Firstly, a massive thanks to @alisnic for starting this (incredibly complicated) lint rule in #2637 !

still a draft. current state:

3x false positives (all todo with refs)

3x false negatives (TS will catch these
13x false negatvies todo with refs
1x false negative TODO

closes  #2637
relates to #2174
Dunqing pushed a commit that referenced this issue Nov 18, 2024
Firstly, a massive thanks to @alisnic for starting this (incredibly complicated) lint rule in #2637 !

still a draft. current state:

3x false positives (all todo with refs)

3x false negatives (TS will catch these
13x false negatvies todo with refs
1x false negative TODO

closes  #2637
relates to #2174
Dunqing pushed a commit that referenced this issue Nov 18, 2024
Firstly, a massive thanks to @alisnic for starting this (incredibly complicated) lint rule in #2637 !

still a draft. current state:

3x false positives (all todo with refs)

3x false negatives (TS will catch these
13x false negatvies todo with refs
1x false negative TODO

closes  #2637
relates to #2174
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

Successfully merging a pull request may close this issue.

3 participants