chore: Add named-use-effect custom eslint rule #36725
Conversation
WalkthroughThe changes in this pull request introduce a new ESLint rule named Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant ESLint
participant Rule
Developer->>ESLint: Run ESLint on code
ESLint->>Rule: Check for `useEffect` usage
alt Valid function
Rule->>Developer: No warning
else Invalid function
Rule->>Developer: Warning: Use named function
end
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
app/client/packages/eslint-plugin/src/named-use-effect/rule.test.ts (2)
8-11: Good start on valid test cases, but let's expand our horizons!Your valid test case correctly demonstrates the proper use of a named function in
useEffect. However, to ensure our rule is robust, we should consider adding more valid test cases.Consider adding these additional valid test cases:
{ code: "useEffect(function myEffect() { /* do something */ }, [dependency])", }, { code: "useEffect(function cleanupEffect() { return () => { /* cleanup */ } }, [])", },These will help us cover more scenarios and ensure our rule works correctly in various situations.
12-21: Well done on the invalid test cases, class!You've correctly identified two key scenarios where our rule should flag an error: anonymous functions and arrow functions in
useEffect. The consistent use of the "useNamedUseEffect" messageId is commendable.To make our tests even more robust, consider adding a test case for a named arrow function, which should be valid:
{ code: "useEffect(const myEffect = () => { /* do something */ }, [])", },This will ensure our rule correctly handles all function syntaxes in
useEffect.app/client/packages/eslint-plugin/src/named-use-effect/rule.ts (1)
13-14: Enhance the error message for clarityRemember, a clear and actionable error message helps developers understand how to fix issues promptly. Consider rephrasing the message to provide guidance, such as: "Avoid using anonymous functions in useEffect; use a named function instead for better readability and debugging."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/packages/eslint-plugin/src/index.ts (1 hunks)
- app/client/packages/eslint-plugin/src/named-use-effect/rule.test.ts (1 hunks)
- app/client/packages/eslint-plugin/src/named-use-effect/rule.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
app/client/packages/eslint-plugin/src/index.ts (3)
7-7: Now, let's look at how we've added this rule to our rulebook.Excellent work adding the new rule to our collection! You've followed the existing pattern perfectly, which helps keep our code organized and easy to understand.
2-13: Class, let's recap what we've learned from these changes.Overall, these changes demonstrate a well-structured approach to expanding our ESLint plugin. By adding the new
namedUseEffectRule, we're encouraging better coding practices in our React components. The consistency in naming, file structure, and configuration is commendable.Remember, clear and consistent code like this makes our project easier to maintain and understand. Keep up the good work!
13-13: Let's examine how we've configured our new rule.Well done on adding the new rule to our recommended configuration! You've maintained consistency with our existing setup, which is crucial for a well-organized codebase. The "warn" level is appropriate for introducing a new rule.
To ensure our new rule is working as intended, let's verify its implementation:
✅ Verification successful
Let's ensure our new ESLint rule is correctly implemented by searching the codebase for any references to
namedUseEffect.
Great observation! It seems
ripgrepdoesn't recognize thetypescriptfile type in our current environment. Let's adjust our search to target.tsand.tsxfiles explicitly to locate any implementations related tonamedUseEffect.
Excellent work! The new
named-use-effectrule has been properly implemented and integrated into our ESLint configuration. Everything looks good and follows our established standards.
namedUseEffectRuleis correctly defined inrule.ts.- The rule is imported and registered in
index.ts.- Comprehensive tests are present in
rule.test.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the new ESLint rule # Test: Search for the rule implementation ast-grep --lang typescript --pattern $'export const namedUseEffectRule = { $$$ create(context) { $$$ } $$$ }'Length of output: 122
Script:
#!/bin/bash # Description: Search for implementations related to 'namedUseEffect' rg 'namedUseEffect' --type typescriptLength of output: 72
Script:
#!/bin/bash # Description: Search for implementations related to 'namedUseEffect' in TypeScript files rg 'namedUseEffect' --glob '*.ts' --glob '*.tsx'Length of output: 627
app/client/packages/eslint-plugin/src/named-use-effect/rule.test.ts (2)
1-4: Good job setting up the test environment, class!You've correctly imported the necessary modules and set up the
RuleTester. This is an excellent foundation for our ESLint rule testing.
6-7: Excellent test suite configuration, students!You've correctly set up the test suite for our "named-use-effect" rule. Remember, proper configuration is key to effective testing.
app/client/packages/eslint-plugin/src/named-use-effect/rule.test.ts
Outdated
Show resolved
Hide resolved
Description
Adds a custom es lint rule that would add a warning when a useEffect is used without a named function inside it
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11252621528
Commit: 334c680
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 09 Oct 2024 10:55:00 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
useEffecthook, promoting better code readability.Tests
namedUseEffectRule, covering both valid and invalid usage scenarios.