From 69325d5113b2010b0780b8c42c48a36bbb4fe61d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Nov 2026 20:28:19 +0000 Subject: [PATCH] v1.63: ESLint, run in CI, and a syntax check of every script Adds an ESLint flat config for the code as it is (classic browser scripts on window.Amber, CommonJS unit checks and specs; 4-space indent, single quotes, semicolons), an `npm run lint` script, and a lint step in the Tests workflow. A small local rule, no-extra-arguments, reports calls that pass more arguments than a function in the same file takes. Fixes what it found, with no change in behaviour: the unused amberRates in computePlanTotals and perKwh in precalculateDailySummaries and channelPeriodBreakdown, demandMonthsHtml's ignored second argument, shadowed names (money, th, sum, d), an unused n in simulateBattery, a raw byte-order mark inside two regex literals, and unused helpers in three specs. tests/unit-syntax.js now starts the unit run: it parses every js/*.js and checks that index.html loads each of them once, with app.js last. Co-Authored-By: Claude Fable 5.2 Claude-Session: https://claude.ai/code/session_016net8VqMzbBJe1deXahjg6 --- .github/workflows/test.yml | 1 + CLAUDE.md | 7 +- README.md | 3 +- docs/ARCHITECTURE.md | 13 +- docs/FRONTEND.md | 1 + docs/REVIEW.md | 10 +- eslint-rules/no-extra-arguments.js | 104 +++ eslint.config.js | 83 +++ index.html | 48 +- js/app.js | 19 +- js/calendar.js | 4 +- js/constants.js | 2 +- js/costs.js | 11 +- js/estimate.js | 2 +- js/import.js | 4 +- js/planner-ui.js | 7 +- js/planner.js | 7 +- js/redispatch.js | 1 + package-lock.json | 1091 +++++++++++++++++++++++++++- package.json | 9 +- scripts/static-server.js | 2 +- tests/nem12.spec.js | 3 +- tests/planner.spec.js | 6 - tests/ui.spec.js | 1 - tests/unit-planner.js | 2 +- tests/unit-syntax.js | 47 ++ tests/unit-time.js | 2 +- 27 files changed, 1379 insertions(+), 111 deletions(-) create mode 100644 eslint-rules/no-extra-arguments.js create mode 100644 eslint.config.js create mode 100644 tests/unit-syntax.js diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ed27256..4389c6c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -24,6 +24,7 @@ jobs: node-version: 22 cache: npm - run: npm ci + - run: npm run lint - run: npx playwright install --with-deps chromium - run: npm test -- --workers=2 - uses: actions/upload-artifact@v4 diff --git a/CLAUDE.md b/CLAUDE.md index 13dd7c3..16fb326 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -12,7 +12,8 @@ A browser-only tool for Australian electricity bills (Amber Electric vs other pl ```bash npm install npm run build:css # after changing any Tailwind class in index.html or js/; commit css/app.css -npm run test:unit # Node unit checks (fast) +npm run lint # ESLint: unused or shadowed variables, undefined names, house style +npm run test:unit # Node unit checks (fast), starting with a syntax check of every js/*.js npm test # unit checks + Playwright (starts scripts/static-server.js on :4173) npx playwright test tests/planner.spec.js --workers=2 # one spec npm run serve # http://127.0.0.1:4173 @@ -40,5 +41,5 @@ If Playwright can't find its browser, point it at an installed Chromium with `la - `js/constants.js`: `Amber.APP_VERSION`; - `tests/unit-time.js`: the version assert. 2. Update `README.md` and `docs/` if behaviour or structure changed. -3. Run `npm run build:css` if classes changed, then `npm test`. -4. Open a pull request. Merge only after the GitHub "Tests" check passes (GitHub Pages serves `main`). +3. Run `npm run build:css` if classes changed, then `npm run lint` and `npm test`. +4. Open a pull request. Merge only after the GitHub "Tests" check passes: it runs the lint and the tests (GitHub Pages serves `main`). diff --git a/README.md b/README.md index 069f2f5..8f6f0e5 100644 --- a/README.md +++ b/README.md @@ -92,8 +92,9 @@ The solar and battery planner. From the last 12 months of meter readings (Amber' ## For developers ```bash -npm install # Tailwind CLI and Playwright +npm install # Tailwind CLI, ESLint and Playwright npm run build:css # css/input.css -> css/app.css (commit the result) +npm run lint # ESLint (eslint.config.js) npm test # unit checks, then Playwright against a local static server npm run test:unit # unit checks only npm run serve # http://127.0.0.1:4173 diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 40c96bf..b0572ba 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -25,7 +25,9 @@ scripts/update-prices.js Downloads AEMO prices into data/prices (run daily by a scripts/update-solar.js Downloads PVGIS weather into data/solar (run by hand) tests/unit-*.js Node unit checks (no framework: assert + console) tests/*.spec.js Playwright end-to-end tests (Chromium) -.github/workflows/ test.yml (tests on PRs and main), update-prices.yml (daily price refresh) +eslint.config.js ESLint settings (npm run lint) +eslint-rules/ The project's own ESLint rules (no-extra-arguments) +.github/workflows/ test.yml (lint and tests on PRs and main), update-prices.yml (daily price refresh) chart.umd.min.js, chartjs-plugin-zoom.min.js, pdf.min.js, pdf.worker.min.js Vendored libraries ``` @@ -218,9 +220,10 @@ Saved JSON is read through `storedJson` / `storedCustomPlans` in `app.js`: a val ## Build, test and CI - `npm run build:css` rebuilds `css/app.css`. Tailwind scans `index.html` and `js/**/*.js` for class names, and `tailwind.config.js` safelists classes that are only built dynamically. Commit `css/app.css` after changing classes. -- **Unit checks**: `tests/unit-*.js`, run with plain `node` (`npm run test:unit`). Each prints `ok:` / `FAIL:` lines and exits non-zero on failure. `unit-planner.js` holds the parity checks: the planner's cost equals Compare's, battery energy balances, finance and ratings. -- **End-to-end**: `tests/*.spec.js` with Playwright (Chromium) against `scripts/static-server.js` on port 4173 (`playwright.config.js`, `reuseExistingServer: true`). Tests mock Amber's API with `page.route('**/api.amber.com.au/**', …)` and build NEM12 files in code. About 54 tests take about 45 s with two workers. -- **CI**: `.github/workflows/test.yml` runs `npm ci`, installs Chromium and runs `npm test -- --workers=2` on every pull request and on pushes to `main`. Failed runs upload `test-results/`. +- **Lint**: `npm run lint` runs ESLint 10 with `eslint.config.js` (a flat config). The files in `js/` are checked as classic browser scripts, and the unit checks, scripts and specs as CommonJS. On top of ESLint's recommended set it reports unused variables, shadowed names, `var`, `==` and a `let` that never changes, and holds the house style (4-space indent, single quotes, semicolons, from `@stylistic/eslint-plugin`). One rule is the project's own: `local/no-extra-arguments` (`eslint-rules/no-extra-arguments.js`) reports a call that passes more arguments than the function takes, for functions and `Amber.*` functions defined in the same file. Rules are tuned to the code rather than the code restyled: the tests keep their own alignment of long asserts, and `preserve-caught-error` is off because network errors are replaced on purpose by plain messages. +- **Unit checks**: `tests/unit-*.js`, run with plain `node` (`npm run test:unit`). Each prints `ok:` / `FAIL:` lines and exits non-zero on failure. `unit-syntax.js` runs first: it parses every `js/*.js` (no other unit script loads the DOM files such as `help.js` and `app.js`) and checks that `index.html` loads each of them once, with `app.js` last. `unit-planner.js` holds the parity checks: the planner's cost equals Compare's, battery energy balances, finance and ratings. +- **End-to-end**: `tests/*.spec.js` with Playwright (Chromium) against `scripts/static-server.js` on port 4173 (`playwright.config.js`, `reuseExistingServer: true`). Tests mock Amber's API with `page.route('**/api.amber.com.au/**', …)` and build NEM12 files in code. About 66 tests take about a minute with two workers. +- **CI**: `.github/workflows/test.yml` runs `npm ci`, `npm run lint`, installs Chromium and runs `npm test -- --workers=2` on every pull request and on pushes to `main`. Failed runs upload `test-results/`. ## Releases @@ -228,4 +231,4 @@ Each change ships as a version: 1. Bump the version in three places: `index.html` (the ``, the "Version X." line in the header, and every `?v=` query string), `js/constants.js` (`Amber.APP_VERSION`), and `tests/unit-time.js` (the version assert). 2. `npm run build:css` if classes changed; update the README and these docs if behaviour changed. -3. Run `npm test`, open a pull request, and merge once the Tests check passes. GitHub Pages serves `main`. +3. Run `npm run lint` and `npm test`, open a pull request, and merge once the Tests check passes. GitHub Pages serves `main`. diff --git a/docs/FRONTEND.md b/docs/FRONTEND.md index 3e27753..ff3945a 100644 --- a/docs/FRONTEND.md +++ b/docs/FRONTEND.md @@ -127,6 +127,7 @@ The app is written for everyday Australian householders: ```bash npm run build:css +npm run lint npm test # or: npx playwright test tests/planner.spec.js npm run serve # then open http://127.0.0.1:4173 ``` diff --git a/docs/REVIEW.md b/docs/REVIEW.md index 57e6341..f7f94d1 100644 --- a/docs/REVIEW.md +++ b/docs/REVIEW.md @@ -7,11 +7,12 @@ Notes for anyone reviewing this codebase: what it must keep doing, where mistake ```bash npm install npm run build:css -npm test # ~10 unit scripts, then ~54 Playwright tests (~45 s with --workers=2) +npm run lint # ESLint +npm test # ~11 unit scripts, then ~66 Playwright tests (~60 s with --workers=2) npm run serve # http://127.0.0.1:4173 ``` -Without an Amber API key, use the "Not with Amber?" path on Start with a NEM12 file. The tests build them in code (`nem12Year()`, `nem12Battery()` in `tests/planner.spec.js`). CI runs the same `npm test` on every pull request (`.github/workflows/test.yml`). +Without an Amber API key, use the "Not with Amber?" path on Start with a NEM12 file. The tests build them in code (`nem12Year()`, `nem12Battery()` in `tests/planner.spec.js`). CI runs `npm run lint` and the same `npm test` on every pull request (`.github/workflows/test.yml`). ## Invariants: what must stay true @@ -44,7 +45,7 @@ These are the properties the tests guard and that users rely on. A change that b - **`app.js` is a monolith.** Tabs, plan form, results, imports, bill check, Free? and settings live in one IIFE sharing a `state` object. Natural seams: the plan form (`createPlanObjectFromForm`, `applyPlanTemplate`, TOU windows), results rendering, imports (CSV, NEM12, local sites), the bill check, the Free? tab and settings. - **HTML as strings.** Most results are template strings set with `innerHTML`, with table markup repeated (`th`/`td` helpers exist only in `planner-ui.js`). It's safe while everything is escaped, but verbose and easy to make inconsistent. -- **No linter, formatter or type checking.** There's no ESLint or Prettier config; the style is 4-space indentation, single quotes and semicolons. JSDoc comments exist on many engine functions but not throughout. +- **No formatter or type checking.** ESLint (`npm run lint`, run in CI) catches unused and shadowed variables, undefined names and calls with too many arguments within a file, and holds the style (4-space indentation, single quotes, semicolons). There's no Prettier, and nothing checks types or calls between files. JSDoc comments exist on many engine functions but not throughout. - **Home-made unit test harness.** `tests/unit-*.js` use their own `assert` and print `ok:`/`FAIL:`. They work, but give no per-test isolation or coverage. - **End-to-end tests depend on IDs and wording.** Many assertions match visible text, so copy edits need test edits too. - **Yearly maintenance is manual.** Each 1 July: the default offers and VDO in `templates.js`, Amber's network tariffs in `networks.js` (FY2026-27 today), Amber's default fixed charges and `AMBER_RATES_CHANGE_DATE` in `constants.js`, rebate and hardware defaults in `PLANNER_FINANCE_DEFAULTS`, and `TEMPLATES_AS_AT`. Market offers drift during the year. @@ -57,7 +58,7 @@ These are the properties the tests guard and that users rely on. A change that b 1. Is it worth splitting `app.js` along the seams above before a frontend refresh, so the refresh doesn't have to touch pricing code? 2. Should HTML building move to small render helpers (or `<template>` elements in `index.html`) to cut repetition, while staying build-free and within the CSP? -3. Would a lint/format setup (ESLint + Prettier, run in CI) be welcome, given the no-build constraint? +3. ESLint now runs in CI. Would a formatter (Prettier) or type checking of the JSDoc (`tsc --checkJs`) be welcome too, given the no-build constraint? 4. Is the planner's battery heuristic good enough, or should it be compared against a simple linear-programming optimum for a few test days to measure how optimistic it is? 5. Should the Solar+? search move to a Web Worker (it runs on the main thread today, yielding with `setTimeout(0)`)? The CSP already allows `worker-src 'self'`. 6. Should the app say explicitly that it's independent of Amber Electric (see FRONTEND.md)? @@ -67,6 +68,7 @@ These are the properties the tests guard and that users rely on. A change that b | Test | Covers | |---|---| +| `unit-syntax.js` | Every `js/*.js` parses as a classic script; `index.html` loads each one once, `app.js` last | | `unit-time.js` | Dates, NEM vs local time, TOU windows, app version | | `unit-bill.js`, `unit-billcheck.js` | Bill reading and line-by-line checks | | `unit-stats.js` | More Stats | diff --git a/eslint-rules/no-extra-arguments.js b/eslint-rules/no-extra-arguments.js new file mode 100644 index 0000000..45e1203 --- /dev/null +++ b/eslint-rules/no-extra-arguments.js @@ -0,0 +1,104 @@ +'use strict'; + +/* + * A local ESLint rule: a call that passes more arguments than the function takes. JavaScript + * drops the extras silently, so they are usually a leftover from an older signature (or a sign + * that the caller expects something the function doesn't do). + * + * It only looks at calls it can be sure about, inside one file: + * - a function declared in that file (`function f(a) {}` or `const f = (a) => {}`), called as `f(…)`; + * - a function put on the namespace in that file (`Amber.f = function (a) {}`), called as `Amber.f(…)`. + * Functions that take `...rest` or read `arguments` are left alone, and so are calls with `...spread`. + */ + +function isFunction(node) { + return !!node && (node.type === 'FunctionDeclaration' || node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression'); +} + +module.exports = { + meta: { + type: 'problem', + docs: { description: 'disallow calling a function with more arguments than it takes' }, + schema: [{ + type: 'object', + properties: { namespaces: { type: 'array', items: { type: 'string' } } }, + additionalProperties: false + }], + messages: { + extra: '\'{{name}}\' takes {{takes}} argument{{s}} but is called with {{given}}.' + } + }, + create(context) { + const sourceCode = context.sourceCode; + const namespaces = new Set((context.options[0] && context.options[0].namespaces) || []); + const members = new Map(); // 'Amber.name' -> function node, or null when assigned twice + const memberCalls = []; + + /** How many arguments the function takes, or null when it takes any number. */ + function arity(fn) { + if (fn.params.some((p) => p.type === 'RestElement')) return null; + if (fn.type !== 'ArrowFunctionExpression') { + const scope = sourceCode.scopeManager.acquire(fn); + const args = scope && scope.set.get('arguments'); + if (args && args.references.length) return null; + } + return fn.params.length; + } + + /** The one function a name always refers to, or null when that can't be known. */ + function localFunction(node) { + let scope = sourceCode.getScope(node); + let variable = null; + while (scope && !variable) { + variable = scope.set.get(node.name) || null; + scope = scope.upper; + } + if (!variable || variable.defs.length !== 1) return null; + const def = variable.defs[0]; + if (variable.references.some((ref) => ref.isWrite() && !ref.init)) return null; + if (def.type === 'FunctionName' && def.node.type === 'FunctionDeclaration') return def.node; + if (def.type === 'Variable' && def.node.id.type === 'Identifier' && isFunction(def.node.init)) return def.node.init; + return null; + } + + function memberName(node) { + if (node.type !== 'MemberExpression' || node.computed) return null; + if (node.object.type !== 'Identifier' || !namespaces.has(node.object.name)) return null; + return `${node.object.name}.${node.property.name}`; + } + + function check(call, fn, name) { + if (call.arguments.some((a) => a.type === 'SpreadElement')) return; + const takes = arity(fn); + if (takes == null || call.arguments.length <= takes) return; + context.report({ + node: call.arguments[takes], + messageId: 'extra', + data: { name, takes, s: takes === 1 ? '' : 's', given: call.arguments.length } + }); + } + + return { + AssignmentExpression(node) { + const name = memberName(node.left); + if (!name) return; + members.set(name, members.has(name) || node.operator !== '=' || !isFunction(node.right) ? null : node.right); + }, + CallExpression(node) { + if (node.callee.type === 'Identifier') { + const fn = localFunction(node.callee); + if (fn) check(node, fn, node.callee.name); + return; + } + const name = memberName(node.callee); + if (name) memberCalls.push({ node, name }); + }, + 'Program:exit'() { + memberCalls.forEach(({ node, name }) => { + const fn = members.get(name); + if (fn) check(node, fn, name); + }); + } + }; + } +}; diff --git a/eslint.config.js b/eslint.config.js new file mode 100644 index 0000000..82a1922 --- /dev/null +++ b/eslint.config.js @@ -0,0 +1,83 @@ +'use strict'; + +/* + * ESLint for the code as it is: classic browser scripts (each js/*.js is an IIFE adding to + * window.Amber, no modules, no build step), Node unit checks and scripts, and Playwright specs. + * It looks for real problems (unused or shadowed variables, unreachable code, undefined names) + * and holds the house style: 4-space indent, single quotes, semicolons. Run: npm run lint + */ +const js = require('@eslint/js'); +const globals = require('globals'); +const stylistic = require('@stylistic/eslint-plugin'); +const noExtraArguments = require('./eslint-rules/no-extra-arguments.js'); + +// The project's own rules (eslint-rules/). +const local = { rules: { 'no-extra-arguments': noExtraArguments } }; + +const rules = { + ...js.configs.recommended.rules, + 'no-unused-vars': ['error', { args: 'after-used', caughtErrors: 'none' }], + 'no-shadow': ['error', { builtinGlobals: false, hoist: 'functions' }], + 'no-unreachable': 'error', + 'local/no-extra-arguments': ['error', { namespaces: ['Amber'] }], + // An error caught from the network is replaced on purpose by a plain message for the householder. + 'preserve-caught-error': 'off', + 'no-var': 'error', + 'prefer-const': ['error', { destructuring: 'all' }], + eqeqeq: ['error', 'always', { null: 'ignore' }], + '@stylistic/indent': ['error', 4, { SwitchCase: 1 }], + '@stylistic/quotes': ['error', 'single', { avoidEscape: true, allowTemplateLiterals: 'always' }], + '@stylistic/semi': ['error', 'always'] +}; + +module.exports = [ + { + // Vendored libraries, built CSS and data are not ours to lint. + ignores: ['node_modules/**', '*.min.js', 'css/**', 'data/**', 'test-results/**', 'playwright-report/**'] + }, + { + files: ['**/*.js'], + plugins: { '@stylistic': stylistic, local }, + linterOptions: { reportUnusedDisableDirectives: 'error' }, + rules + }, + { + // The app: classic scripts sharing window.Amber. The same files are `require`d by the Node + // unit checks, hence `module` and `globalThis`. Chart and pdfjsLib are the vendored libraries. + files: ['js/**/*.js'], + languageOptions: { + ecmaVersion: 2022, + sourceType: 'script', + globals: { + ...globals.browser, + Chart: 'readonly', + pdfjsLib: 'readonly', + module: 'readonly' + } + } + }, + { + files: ['tests/unit-*.js', 'scripts/**/*.js', 'eslint-rules/**/*.js', '*.config.js'], + languageOptions: { + ecmaVersion: 2022, + sourceType: 'commonjs', + globals: { ...globals.node } + } + }, + { + // Playwright specs run in Node, with page.evaluate callbacks that run in the page. + files: ['tests/**/*.spec.js'], + languageOptions: { + ecmaVersion: 2022, + sourceType: 'commonjs', + globals: { ...globals.node, ...globals.browser, Amber: 'readonly', Chart: 'readonly' } + } + }, + { + // The tests line up long assert conditions and tables of cases their own way. + files: ['tests/**/*.js'], + rules: { + '@stylistic/indent': ['error', 4, { SwitchCase: 1, ArrayExpression: 'off', CallExpression: { arguments: 'off' } }] + } + } +]; diff --git a/index.html b/index.html index 163ecb6..de111a5 100644 --- a/index.html +++ b/index.html @@ -4,9 +4,9 @@ <meta charset="UTF-8"> <meta name="viewport" content="width=device-width, initial-scale=1.0"> <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'; style-src 'self'; connect-src 'self' https://api.amber.com.au; img-src 'self' data:; font-src 'self'; worker-src 'self'; base-uri 'self'; form-action 'self'"> - <title>Amber Compare & Export v1.62 + Amber Compare & Export v1.63 - + Skip to the tabs @@ -19,7 +19,7 @@

Amber Compare & Export

See what your electricity really costs on Amber, check an Amber bill, and find out whether another plan (or Amber) would be cheaper for you.

-

Free and private: everything runs in this browser and nothing is uploaded. Version 1.62.

+

Free and private: everything runs in this browser and nothing is uploaded. Version 1.63.