diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index b0598b7..7218b83 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -22,3 +22,4 @@ jobs: node-version: ${{ matrix.node }} - run: npm install - run: npm run lint + - run: npm test diff --git a/README.md b/README.md index ec1c2f9..e21ca37 100644 --- a/README.md +++ b/README.md @@ -11,6 +11,7 @@ This package provides ESLint rules and configurations for **Hubspot Marketing We - [Stylelint Setup](#stylelint-setup) - [Cypress Setup](#cypress-setup) - [Accessibility Testing](#accessibility-testing-optional) +- [Custom Rules](#custom-rules) - [Where to use it](#where-to-use-it) - [Using the Prettier Scripts](#using-the-prettier-scripts) - [Contributing](#contributing) @@ -158,6 +159,16 @@ This package includes a shared accessibility testing setup using [cypress-axe](h `cy.checkAccessibility()` adds the `high-contrast` class to `body`, runs WCAG 2.2 Level AA rules only, and logs each violation with its id, help text, impact, element targets, and help URL. TypeScript types are included — no `tsconfig.json` changes required. +## Custom Rules + +This package ships a `hs-web-team` ESLint plugin with custom rules that are active automatically when you use `wtConfig` or `wtBrowserConfig` — no extra setup needed. + +| Rule | Severity | Summary | +|------|----------|---------| +| `hs-web-team/no-reduce-accumulator-copy` | error | Disallows O(n²) accumulator copies (spread, concat) inside `reduce`/`reduceRight` | + +For examples and remediation guidance, see [examples/custom-rules.md](./examples/custom-rules.md). + ## Where to use it This package provides multiple configurations: diff --git a/bin/check-peer-deps.cjs b/bin/check-peer-deps.cjs index a35f27d..fefc2e3 100644 --- a/bin/check-peer-deps.cjs +++ b/bin/check-peer-deps.cjs @@ -14,7 +14,7 @@ if (!projectRoot || projectRoot === path.resolve(__dirname, '..')) { const ownPkg = require('../package.json'); const stylelintPeers = Object.entries(ownPkg.peerDependencies || {}).filter(([name]) => - name.startsWith('stylelint') + name.startsWith('stylelint'), ); // Extracts the minimum required major version from a semver range like ^17.0.0 or >=17.1.1. diff --git a/browser.js b/browser.js index d379599..8bf93e6 100644 --- a/browser.js +++ b/browser.js @@ -4,6 +4,7 @@ import tseslint from 'typescript-eslint'; import reactPlugin from 'eslint-plugin-react'; import reactHooksPlugin from 'eslint-plugin-react-hooks'; import jsxA11yPlugin from 'eslint-plugin-jsx-a11y'; +import { hsWebTeamPlugin } from './plugins/hs-web-team/index.js'; // Base rules adapted from the browser config const baseRules = { @@ -63,7 +64,6 @@ const commonIgnores = [ '**/build/**', '**/.next/**', '**/coverage/**', - 'eslint.config.js', ]; export default [ @@ -117,6 +117,7 @@ export default [ ...reactRules, }, }, + hsWebTeamPlugin.configs.recommended, // TypeScript config ...tseslint.configs.recommended.map(config => ({ ...config, diff --git a/eslint.config.js b/eslint.config.js new file mode 100644 index 0000000..6459fc6 --- /dev/null +++ b/eslint.config.js @@ -0,0 +1,16 @@ +import wtConfig from './index.js'; + +export default [ + ...wtConfig, + // Cypress utility files use globals (Cypress, cy) injected by the Cypress + // runtime — declare them so no-undef doesn't false-positive on this directory. + { + files: ['cypress/**'], + languageOptions: { + globals: { + Cypress: 'readonly', + cy: 'readonly', + }, + }, + }, +]; diff --git a/examples/custom-rules.md b/examples/custom-rules.md new file mode 100644 index 0000000..9a1b431 --- /dev/null +++ b/examples/custom-rules.md @@ -0,0 +1,59 @@ +# Custom Rules (`hs-web-team` plugin) + +This package bundles a small set of custom ESLint rules under the `hs-web-team` plugin. They are active automatically when you spread `wtConfig` or `wtBrowserConfig` — no extra configuration required. + +--- + +## `hs-web-team/no-reduce-accumulator-copy` ❌ error + +**Disallows O(n²) accumulator copies inside `reduce`/`reduceRight` callbacks.** + +Each iteration of `reduce` creates an entirely new object or array, making the total work proportional to n². This is rarely intentional and degrades quickly as the input grows. + +### What triggers it + +```js +// ❌ Object spread — copies every key on every iteration +const byId = items.reduce((acc, item) => ({ ...acc, [item.id]: item }), {}); + +// ❌ Array spread — copies every element on every iteration +const doubled = items.reduce((acc, n) => [...acc, n * 2], []); + +// ❌ acc.concat() — same problem, different syntax +const flat = items.reduce((acc, arr) => acc.concat(arr), []); +``` + +### What to do instead + +```js +// ✅ Object.groupBy / Object.fromEntries for grouping/indexing +const byId = Object.fromEntries(items.map(item => [item.id, item])); + +// ✅ for...of with in-place mutation for accumulation +const byId = {}; +for (const item of items) { + byId[item.id] = item; +} + +// ✅ flatMap for flat-map patterns +const doubled = items.flatMap(n => [n * 2]); + +// ✅ flat() for flattening +const flat = items.flat(); +``` + +### When `reduce` is fine + +The rule only fires when the **accumulator itself** is copied. Reads from the accumulator, or spreading non-accumulator values into it, are allowed: + +```js +// ✅ Spreading a non-accumulator value into the result +const merged = items.reduce((acc, item) => ({ ...acc, ...item.overrides }), base); +// ^^^^^^^^^^^^^^ not the accumulator — fine + +// ✅ Accumulator mutation with no copy +const counts = items.reduce((acc, item) => { + acc[item.type] = (acc[item.type] ?? 0) + 1; + return acc; +}, {}); +``` diff --git a/index.js b/index.js index 1c14b07..9257af9 100644 --- a/index.js +++ b/index.js @@ -1,6 +1,7 @@ import js from '@eslint/js'; import globals from 'globals'; import tseslint from 'typescript-eslint'; +import { hsWebTeamPlugin } from './plugins/hs-web-team/index.js'; // Base rules for all JavaScript files const baseRules = { @@ -51,7 +52,6 @@ const commonIgnores = [ '**/.serverless/**', '**/.webpack/**', '**/dist/**', - 'eslint.config.js', ]; export default [ @@ -68,6 +68,7 @@ export default [ }, rules: baseRules, }, + hsWebTeamPlugin.configs.recommended, // TypeScript config - restrict to TypeScript files only ...tseslint.configs.recommended.map(config => ({ ...config, diff --git a/package.json b/package.json index 65cb8a8..61bf4ac 100644 --- a/package.json +++ b/package.json @@ -2,6 +2,19 @@ "name": "@hs-web-team/eslint-config-node", "version": "4.2.2", "description": "HubSpot Marketing WebTeam shared configurations for ESLint, Prettier, Stylelint, and Cypress", + "files": [ + ".prettierrc.json", + ".stylelintrc.json", + "bin/", + "browser.js", + "cypress.config.cjs", + "cypress.config.d.ts", + "cypress/", + "docs/", + "examples/", + "index.js", + "plugins/" + ], "main": "index.js", "type": "module", "exports": { @@ -31,8 +44,8 @@ }, "scripts": { "postinstall": "node bin/check-peer-deps.cjs", - "lint": "npx eslint -c ./index.js *.js --fix", - "test": "echo \"Error: no test specified\" && exit 1", + "lint": "npx eslint . --fix", + "test": "node --test", "prepare": "git rev-parse --git-dir > /dev/null 2>&1 && git config core.hooksPath .githooks || true" }, "engines": { diff --git a/plugins/hs-web-team/index.js b/plugins/hs-web-team/index.js new file mode 100644 index 0000000..8e1593f --- /dev/null +++ b/plugins/hs-web-team/index.js @@ -0,0 +1,18 @@ +import { noReduceAccumulatorCopy } from './rules/no-reduce-accumulator-copy.js'; + +export const hsWebTeamPlugin = { + rules: { + 'no-reduce-accumulator-copy': noReduceAccumulatorCopy, + }, +}; + +// Self-reference allows the plugin to register itself via configs.recommended, +// following the same pattern as typescript-eslint and eslint-plugin-react. +hsWebTeamPlugin.configs = { + recommended: { + plugins: { 'hs-web-team': hsWebTeamPlugin }, + rules: { + 'hs-web-team/no-reduce-accumulator-copy': 'error', + }, + }, +}; diff --git a/plugins/hs-web-team/rules/no-reduce-accumulator-copy.js b/plugins/hs-web-team/rules/no-reduce-accumulator-copy.js new file mode 100644 index 0000000..f12f38b --- /dev/null +++ b/plugins/hs-web-team/rules/no-reduce-accumulator-copy.js @@ -0,0 +1,90 @@ +const RECOMMENDATION = + 'Prefer a for...of loop with in-place mutation, or a purpose-built method (flatMap, Object.groupBy, flat) if it fits your pattern.'; + +function isReduceCall(node) { + if (!node) return false; + + const callee = node.callee; + if (!callee || !callee.property || !callee.property.name) { + return false; + } + + return ( + callee.property.name === 'reduce' || callee.property.name === 'reduceRight' + ); +} + +function isReduceAccumulator(def) { + return ( + def && + def.type === 'Parameter' && + def.node.parent && + isReduceCall(def.node.parent) && + def.node.params && + def.node.params.length && + // def.name is the Identifier AST node for the definition; this identity + // check confirms it is the first parameter (the accumulator), not a later one. + def.name === def.node.params[0] + ); +} + +export const noReduceAccumulatorCopy = { + meta: { + type: 'problem', + docs: { + description: + 'Disallow O(n²) accumulator copies (spread, concat) inside reduce/reduceRight callbacks', + recommended: true, + }, + messages: { + AccumulatorSpread: `Spreading the reduce accumulator is O(n²). ${RECOMMENDATION}`, + AccumulatorShallowCopy: `Calling .{{ method }}() on the reduce accumulator is O(n²). ${RECOMMENDATION}`, + }, + schema: [], + }, + + create(context) { + const { sourceCode } = context; + + function checkIsAccumulatorSpread(node) { + if (!node.argument || node.argument.type !== 'Identifier') return; + + const scope = sourceCode.getScope(node); + const spreadee = scope.set.get(node.argument.name); + const spreadeeDef = + spreadee && spreadee.defs && spreadee.defs.length && spreadee.defs[0]; + + if (isReduceAccumulator(spreadeeDef)) { + context.report({ node, messageId: 'AccumulatorSpread' }); + } + } + + function checkIsAccumulatorShallowCopy(node) { + if ( + node.callee.type !== 'MemberExpression' || + !node.callee.property || + !node.callee.object || + node.callee.object.type !== 'Identifier' + ) { + return; + } + + const method = node.callee.property.name; + if (method !== 'concat') return; + + const scope = sourceCode.getScope(node); + const variable = scope.set.get(node.callee.object.name); + const definition = + variable && variable.defs && variable.defs.length && variable.defs[0]; + + if (isReduceAccumulator(definition)) { + context.report({ node, messageId: 'AccumulatorShallowCopy', data: { method } }); + } + } + + return { + SpreadElement: checkIsAccumulatorSpread, + CallExpression: checkIsAccumulatorShallowCopy, + }; + }, +}; diff --git a/tests/plugins/hs-web-team/no-reduce-accumulator-copy.test.js b/tests/plugins/hs-web-team/no-reduce-accumulator-copy.test.js new file mode 100644 index 0000000..ef31e05 --- /dev/null +++ b/tests/plugins/hs-web-team/no-reduce-accumulator-copy.test.js @@ -0,0 +1,146 @@ +import { describe, it } from 'node:test'; +import { RuleTester } from 'eslint'; +import { noReduceAccumulatorCopy as rule } from '../../../plugins/hs-web-team/rules/no-reduce-accumulator-copy.js'; + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'module', + }, +}); + +describe('no-reduce-accumulator-copy', () => { + describe('valid', () => { + it('does not flag spreading a non-accumulator variable inside reduce', () => { + ruleTester.run('no-reduce-accumulator-copy', rule, { + valid: [ + // Spreading the current item (object), not the accumulator + { code: 'items.reduce((acc, item) => ({ ...item, extra: 1 }), {})' }, + // Spreading the current item (array), not the accumulator + { code: 'items.reduce((acc, item) => [...item], [])' }, + ], + invalid: [], + }); + }); + + it('does not flag Object.fromEntries + flatMap (a preferred alternative)', () => { + ruleTester.run('no-reduce-accumulator-copy', rule, { + valid: [ + { + code: 'Object.fromEntries(items.flatMap(item => [[item.key, item.value]]))', + }, + ], + invalid: [], + }); + }); + + it('does not flag a for...of loop with push (a preferred alternative)', () => { + ruleTester.run('no-reduce-accumulator-copy', rule, { + valid: [ + { + code: ` + const result = []; + for (const item of items) { + result.push(item.value); + } + `, + }, + ], + invalid: [], + }); + }); + + it('does not flag in-place mutation of the accumulator', () => { + ruleTester.run('no-reduce-accumulator-copy', rule, { + valid: [ + { + code: 'items.reduce((acc, item) => { acc[item.key] = item.value; return acc; }, {})', + }, + { + code: 'items.reduce((acc, item) => { acc.push(item); return acc; }, [])', + }, + ], + invalid: [], + }); + }); + + it('does not flag spreading a variable with the same name outside a reduce', () => { + ruleTester.run('no-reduce-accumulator-copy', rule, { + valid: [ + { + code: ` + const acc = { a: 1 }; + const result = { ...acc, b: 2 }; + `, + }, + ], + invalid: [], + }); + }); + }); + + describe('invalid', () => { + it('flags object spread on the accumulator', () => { + ruleTester.run('no-reduce-accumulator-copy', rule, { + valid: [], + invalid: [ + { + code: 'items.reduce((acc, item) => ({ ...acc, [item.key]: item.value }), {})', + errors: [{ messageId: 'AccumulatorSpread' }], + }, + ], + }); + }); + + it('flags array spread on the accumulator', () => { + ruleTester.run('no-reduce-accumulator-copy', rule, { + valid: [], + invalid: [ + { + code: 'items.reduce((acc, item) => [...acc, item], [])', + errors: [{ messageId: 'AccumulatorSpread' }], + }, + ], + }); + }); + + it('flags acc.concat() on the accumulator', () => { + ruleTester.run('no-reduce-accumulator-copy', rule, { + valid: [], + invalid: [ + { + code: 'items.reduce((acc, item) => acc.concat(item), [])', + errors: [{ messageId: 'AccumulatorShallowCopy' }], + }, + ], + }); + }); + + it('flags object spread on the accumulator inside reduceRight', () => { + ruleTester.run('no-reduce-accumulator-copy', rule, { + valid: [], + invalid: [ + { + code: 'items.reduceRight((acc, item) => ({ ...acc, [item.key]: item.value }), {})', + errors: [{ messageId: 'AccumulatorSpread' }], + }, + ], + }); + }); + + it('flags multiple accumulator copies in the same reduce', () => { + ruleTester.run('no-reduce-accumulator-copy', rule, { + valid: [], + invalid: [ + { + code: 'items.reduce((acc, item) => item.flag ? { ...acc, [item.key]: item.value } : acc.concat(item), [])', + errors: [ + { messageId: 'AccumulatorSpread' }, + { messageId: 'AccumulatorShallowCopy' }, + ], + }, + ], + }); + }); + }); +});