diff --git a/docs/eslint-rules.md b/docs/eslint-rules.md new file mode 100644 index 0000000..eea3a52 --- /dev/null +++ b/docs/eslint-rules.md @@ -0,0 +1,61 @@ +# ESLint Rules + +This document describes the custom ESLint rules used in the UI Kit project. + +## `ui-kit-rules/named-effect-with-cleanup` + +Enforces that all `useEffect` hooks: + +1. Use a named function (not an arrow function or anonymous function) +2. Include a cleanup function + +### Why? + +- **Named functions** make it easier to understand what an effect is doing at a glance +- **Cleanup functions** are essential for preventing memory leaks and side effects when components unmount + +### Good Examples + +```jsx +// Good: Named function with cleanup +useEffect( + function setupSubscription() { + const subscription = api.subscribe(); + return function cleanupSubscription() { + subscription.unsubscribe(); + }; + }, + [api], +); + +// Good: Named external function with cleanup +function setupListeners() { + window.addEventListener("resize", handleResize); + return function cleanupListeners() { + window.removeEventListener("resize", handleResize); + }; +} +useEffect(setupListeners, []); +``` + +### Bad Examples + +```jsx +// Bad: Arrow function +useEffect(() => { + document.title = "New Page"; + // Missing cleanup +}, []); + +// Bad: Anonymous function +useEffect(function () { + const timer = setInterval(tick, 1000); + // Should return a cleanup function to clear the interval +}, []); + +// Bad: Arrow function with cleanup (still bad because of arrow function) +useEffect(() => { + const timer = setInterval(tick, 1000); + return () => clearInterval(timer); +}, []); +``` diff --git a/docs/project_plan.md b/docs/project_plan.md index 763195b..c72cb82 100644 --- a/docs/project_plan.md +++ b/docs/project_plan.md @@ -45,7 +45,7 @@ A pragmatic breakdown into **four one‑week sprints** plus a preparatory **Spri | 2.1 | Add NumberInput, Select, Checkbox, RadioGroup components. | Unit & a11y tests pass; form story displays all. | ✓ | | 2.2 | Integrate **React Hook Form + Zod**; create `FormGrid` + `FormGroup`. | Story "Form Example" submits & reports validation errors in Storybook interaction test. | ✓ | | 2.3 | Implement Zustand session store skeleton with dark‑mode flag. | Vitest verifies default state + setter actions. | ✓ | -| 2.4 | ESLint rule enforcing named `useEffect` & cleanup. | Failing example in test repo triggers lint error; real code passes. | | +| 2.4 | ESLint rule enforcing named `useEffect` & cleanup. | Failing example in test repo triggers lint error; real code passes. | PR | | 2.5 | Extend CI to run axe‑core on all stories. | Pipeline fails if any new a11y violations introduced. | | --- diff --git a/docs/task-planning/eslint-rule-useeffect.md b/docs/task-planning/eslint-rule-useeffect.md new file mode 100644 index 0000000..8d2ecb3 --- /dev/null +++ b/docs/task-planning/eslint-rule-useeffect.md @@ -0,0 +1,37 @@ +# ESLint Rule for useEffect - Task Planning + +## Task Description + +| Task Description | DoD (Definition of Done) | Status | +| --------------------------------------------------------------------------- | ------------------------------------------------------------------ | -------- | +| Implement ESLint rule that enforces named `useEffect` and cleanup functions | Failing example in test repo triggers lint error; real code passes | Complete | + +## Implementation Plan + +1. Create a custom ESLint rule that: + + - Detects React's `useEffect` hook usage + - Verifies that the first argument is a named function (not an inline arrow function) + - Ensures the function includes a return statement with a cleanup function + +2. Test the rule with: + + - Examples that should pass (named function with cleanup) + - Examples that should fail (inline function or missing cleanup) + +3. Add the rule to the project's ESLint configuration + +4. Document the rule and usage patterns in a README or documentation + +## Technical Details + +- We'll need to create a custom ESLint plugin +- The rule will use AST parsing to detect useEffect usage patterns +- The configuration will be added to the existing ESLint setup + +## Timeline + +- Setup custom ESLint plugin: 1 hour +- Implement rule logic: 2 hours +- Test and verify rule functionality: 1 hour +- Documentation: 30 minutes diff --git a/eslint-plugin-ui-kit-rules/test-examples.js b/eslint-plugin-ui-kit-rules/test-examples.js new file mode 100644 index 0000000..1f23d19 --- /dev/null +++ b/eslint-plugin-ui-kit-rules/test-examples.js @@ -0,0 +1,63 @@ +// Bad examples - should trigger errors + +import { useEffect } from 'react'; + +// Bad: Arrow function without cleanup +const BadArrowNoCleanup = () => { + useEffect(() => { + console.log('Effect running'); + // Missing cleanup function + }, []); +}; + +// Bad: Anonymous function without cleanup +const BadAnonymousNoCleanup = () => { + useEffect(function () { + console.log('Effect running'); + // Missing cleanup function + }, []); +}; + +// Bad: Arrow function with cleanup +const BadArrowWithCleanup = () => { + useEffect(() => { + console.log('Effect running'); + return () => { + console.log('Cleanup running'); + }; + }, []); +}; + +// Bad: Anonymous function with cleanup +const BadAnonymousWithCleanup = () => { + useEffect(function () { + console.log('Effect running'); + return () => { + console.log('Cleanup running'); + }; + }, []); +}; + +// Good examples - should pass + +// Good: Named function with cleanup +const GoodNamedWithCleanup = () => { + useEffect(function setupEffect() { + console.log('Effect running'); + return function cleanupEffect() { + console.log('Cleanup running'); + }; + }, []); +}; + +// Good: External named function with cleanup +const GoodExternalNamedFunction = () => { + function setupEffect() { + console.log('Effect running'); + return function cleanupEffect() { + console.log('Cleanup running'); + }; + } + + useEffect(setupEffect, []); +}; \ No newline at end of file diff --git a/eslint.config.js b/eslint.config.js index f93eb28..d75db38 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -2,6 +2,7 @@ import js from '@eslint/js'; import tseslint from 'typescript-eslint'; import reactHooks from 'eslint-plugin-react-hooks'; import reactRefresh from 'eslint-plugin-react-refresh'; +import uiKitRules from './packages/eslint-plugin-ui-kit-rules/index.js'; export default [ // global ignores – applied before other configs @@ -28,7 +29,8 @@ export default [ }, plugins: { 'react-hooks': reactHooks, - 'react-refresh': reactRefresh + 'react-refresh': reactRefresh, + 'ui-kit-rules': uiKitRules }, rules: { 'react-hooks/rules-of-hooks': 'error', @@ -38,6 +40,8 @@ export default [ { allowConstantExport: true } ], '@typescript-eslint/no-require-imports': 'off', + // Custom rules + 'ui-kit-rules/named-effect-with-cleanup': 'error' // 'no-undef': 'off' // Let's remove this for now and see if globals cover it. } } diff --git a/packages/eslint-plugin-ui-kit-rules/README.md b/packages/eslint-plugin-ui-kit-rules/README.md new file mode 100644 index 0000000..88a7b83 --- /dev/null +++ b/packages/eslint-plugin-ui-kit-rules/README.md @@ -0,0 +1,76 @@ +# UI Kit ESLint Rules + +This package contains custom ESLint rules for the UI Kit project. + +## Rules + +### `ui-kit-rules/named-effect-with-cleanup` + +Enforces that all `useEffect` hooks: + +1. Use a named function (not an arrow function or anonymous function) +2. Include a cleanup function + +#### Why? + +- **Named functions** make it easier to understand what an effect is doing at a glance +- **Cleanup functions** are essential for preventing memory leaks and side effects when components unmount + +#### Good Examples + +```jsx +// Good: Named function with cleanup +useEffect( + function setupSubscription() { + const subscription = api.subscribe(); + return function cleanupSubscription() { + subscription.unsubscribe(); + }; + }, + [api], +); + +// Good: Named external function with cleanup +function setupListeners() { + window.addEventListener("resize", handleResize); + return function cleanupListeners() { + window.removeEventListener("resize", handleResize); + }; +} +useEffect(setupListeners, []); +``` + +#### Bad Examples + +```jsx +// Bad: Arrow function +useEffect(() => { + document.title = "New Page"; + // Missing cleanup +}, []); + +// Bad: Anonymous function +useEffect(function () { + const timer = setInterval(tick, 1000); + // Should return a cleanup function to clear the interval +}, []); + +// Bad: Arrow function with cleanup (still bad because of arrow function) +useEffect(() => { + const timer = setInterval(tick, 1000); + return () => clearInterval(timer); +}, []); +``` + +## Usage + +Add this rule to your ESLint configuration: + +```js +{ + "plugins": ["ui-kit-rules"], + "rules": { + "ui-kit-rules/named-effect-with-cleanup": "error" + } +} +``` diff --git a/packages/eslint-plugin-ui-kit-rules/index.js b/packages/eslint-plugin-ui-kit-rules/index.js new file mode 100644 index 0000000..88d92ce --- /dev/null +++ b/packages/eslint-plugin-ui-kit-rules/index.js @@ -0,0 +1,82 @@ +export default { + rules: { + 'named-effect-with-cleanup': { + meta: { + type: 'suggestion', + docs: { + description: 'Enforce named functions in useEffect with cleanup', + category: 'Best Practices', + recommended: true, + }, + messages: { + requireNamedFunction: 'useEffect should use a named function instead of an arrow or anonymous function', + requireCleanup: 'useEffect should return a cleanup function', + }, + schema: [], // no options + }, + create(context) { + return { + // Look for useEffect calls + CallExpression(node) { + // Check if this is a useEffect call + if ( + node.callee.type === 'Identifier' && + node.callee.name === 'useEffect' + ) { + const [effectCallback] = node.arguments; + + // Skip if there are no arguments + if (!effectCallback) return; + + // Check if the callback is an arrow function or anonymous function expression + // A named function is either: + // - a FunctionExpression with an id + // - an Identifier (reference to a previously defined function) + + // Check for arrow functions or anonymous functions + if ( + (effectCallback.type === 'ArrowFunctionExpression') || + (effectCallback.type === 'FunctionExpression' && !effectCallback.id) + ) { + context.report({ + node: effectCallback, + messageId: 'requireNamedFunction', + }); + } + + // Check for cleanup function + // For direct function expressions (anonymous or named) + if ( + (effectCallback.type === 'FunctionExpression' || + effectCallback.type === 'ArrowFunctionExpression') + ) { + const { body } = effectCallback; + + // For block bodies (with curly braces) + if (body.type === 'BlockStatement') { + const hasReturnStatement = body.body.some(statement => { + return statement.type === 'ReturnStatement' && statement.argument !== null; + }); + + if (!hasReturnStatement) { + context.report({ + node: effectCallback, + messageId: 'requireCleanup', + }); + } + } + // For implicit returns in arrow functions + else if (body.type !== 'ReturnStatement' && body.type !== 'FunctionExpression') { + context.report({ + node: effectCallback, + messageId: 'requireCleanup', + }); + } + } + } + } + }; + } + } + } +}; \ No newline at end of file diff --git a/packages/eslint-plugin-ui-kit-rules/package.json b/packages/eslint-plugin-ui-kit-rules/package.json new file mode 100644 index 0000000..239d071 --- /dev/null +++ b/packages/eslint-plugin-ui-kit-rules/package.json @@ -0,0 +1,10 @@ +{ + "name": "@org/eslint-plugin-ui-kit-rules", + "version": "1.0.0", + "description": "Custom ESLint rules for UI Kit", + "main": "index.js", + "type": "module", + "author": "", + "license": "ISC", + "dependencies": {} +} diff --git a/packages/ui-kit/src/hooks/useTheme.ts b/packages/ui-kit/src/hooks/useTheme.ts index 1389073..8ea7bf6 100644 --- a/packages/ui-kit/src/hooks/useTheme.ts +++ b/packages/ui-kit/src/hooks/useTheme.ts @@ -12,7 +12,7 @@ export function useTheme() { const [systemPrefersDark, setSystemPrefersDark] = useState(false); // Detect system preference - useEffect(() => { + useEffect(function setupSystemPreference() { // Check for system dark mode preference const mediaQuery = window.matchMedia('(prefers-color-scheme: dark)'); setSystemPrefersDark(mediaQuery.matches); @@ -23,7 +23,9 @@ export function useTheme() { }; mediaQuery.addEventListener('change', handler); - return () => mediaQuery.removeEventListener('change', handler); + return function cleanupSystemPreference() { + mediaQuery.removeEventListener('change', handler); + }; }, []); // Helper to sync with system preference @@ -32,12 +34,18 @@ export function useTheme() { }; // Set theme class on document - useEffect(() => { + useEffect(function applyThemeClass() { if (isDarkMode) { document.documentElement.classList.add('dark'); } else { document.documentElement.classList.remove('dark'); } + + // Cleanup function to remove theme class if component unmounts + return function cleanupThemeClass() { + // This is a no-op in this case, but we include it to comply with our ESLint rule + // In a real-world scenario, you might want to restore the previous theme + }; }, [isDarkMode]); return { diff --git a/packages/ui-kit/src/providers/ThemeProvider.tsx b/packages/ui-kit/src/providers/ThemeProvider.tsx index 0a42bcc..ea17e88 100644 --- a/packages/ui-kit/src/providers/ThemeProvider.tsx +++ b/packages/ui-kit/src/providers/ThemeProvider.tsx @@ -25,10 +25,16 @@ export function ThemeProvider({ const { isDarkMode, syncWithSystemPreference } = useTheme(); // Initialize theme based on system preferences if enabled - useEffect(() => { + useEffect(function setupSystemSync() { if (syncWithSystemOnMount) { syncWithSystemPreference(); } + + // Return cleanup function + return function cleanupSystemSync() { + // This is a no-op, but included to comply with our ESLint rule + // In a real-world scenario, you might want to perform cleanup tasks + }; }, [syncWithSystemOnMount, syncWithSystemPreference]); return (