Compare commits
1 commit
main
...
convoy/lin
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
1f100d1831 |
2 changed files with 185 additions and 0 deletions
176
.convoys/lint-against-cjs-in-esm-scripts.md
Normal file
176
.convoys/lint-against-cjs-in-esm-scripts.md
Normal file
|
|
@ -0,0 +1,176 @@
|
||||||
|
---
|
||||||
|
name: lint-against-cjs-in-esm-scripts
|
||||||
|
classification: hygiene
|
||||||
|
success_metric: future helper scripts that re-introduce CJS `require()` calls under `package.json` "type": "module" fail at lint time, not at first execution
|
||||||
|
status: open
|
||||||
|
created: 2026-05-26
|
||||||
|
---
|
||||||
|
|
||||||
|
# lint-against-cjs-in-esm-scripts (P3 polish — parent-owned)
|
||||||
|
|
||||||
|
**Priority:** P3 polish (one-line ESLint rule; no architect required)
|
||||||
|
**Convoy owner:** parent
|
||||||
|
**Opened:** 2026-05-26
|
||||||
|
|
||||||
|
## Background — the recurring bug pattern
|
||||||
|
|
||||||
|
Since `bump-next-js` flipped `package.json` to `"type": "module"`,
|
||||||
|
any helper script under `scripts/` that uses CJS `require()` throws
|
||||||
|
`ReferenceError: require is not defined` on Node 22.x at first run.
|
||||||
|
The same bug has now bitten the repo twice in two convoys:
|
||||||
|
|
||||||
|
1. **`drop-public-setup` Brief 2** (commit `b63b509`, 2026-05-23):
|
||||||
|
`scripts/setup-neon-db.js` was still CJS post-`bump-next-js`; `npm
|
||||||
|
run setup-db` was silently broken until Brief 2 swept it to ESM
|
||||||
|
imports. The convoy retro called this out as "the seed script
|
||||||
|
silently stopped executing after `bump-next-js`."
|
||||||
|
2. **`fix-reset-db-script` Brief 1** (commit `3ab9bf8`, PR #25,
|
||||||
|
2026-05-26): three `require()` calls in `scripts/reset-db.js`
|
||||||
|
(lines 10, 12, 142) — same bug, same blast radius (`npm run
|
||||||
|
reset-db` throws `ReferenceError`), same fix shape (verbatim
|
||||||
|
mirror of post-`drop-public-setup` `setup-neon-db.js`).
|
||||||
|
|
||||||
|
Both bugs were caught at first run, not at lint time. A small
|
||||||
|
ESLint rule scoped to `scripts/**/*.js` would have caught both at
|
||||||
|
PR time and is cheap insurance against a third recurrence.
|
||||||
|
|
||||||
|
## Design decision — `scripts/**` only (NOT all `.js`)
|
||||||
|
|
||||||
|
Two reasonable scopes:
|
||||||
|
|
||||||
|
- **`scripts/**/*.js` (chosen):** matches the actual blast radius —
|
||||||
|
every observed instance of the bug has been in a helper script.
|
||||||
|
Per-file-block override in `eslint.config.mjs` via a second flat-
|
||||||
|
config entry. Zero impact on `pages/api/**` (already correctly
|
||||||
|
ESM-imported throughout) and zero impact on the root `*.config.js`
|
||||||
|
files (which are intentionally CJS-shaped and which the next-config
|
||||||
|
base rules already handle correctly).
|
||||||
|
- **All `.js` files at repo root (rejected):** broader-than-necessary
|
||||||
|
blast radius. `pages/api/**` already uses ESM `import` everywhere
|
||||||
|
(`add-rate-limiting`, `cors-tighten`, and `add-route` skill all
|
||||||
|
verified this in the last three months). A repo-wide ban would
|
||||||
|
produce zero true positives outside `scripts/**` today and would
|
||||||
|
risk breaking config-file shapes that legitimately use CJS
|
||||||
|
(`postcss.config.js`, `tailwind.config.js` are flagged by
|
||||||
|
`import/no-anonymous-default-export` today but read CJS-style
|
||||||
|
exports under the hood — see also Gotcha #9 + #10).
|
||||||
|
|
||||||
|
The scoped rule is a 7-line flat-config block; the broader rule
|
||||||
|
would require explicit allowlist for every config file, which is
|
||||||
|
strictly more code and more maintenance.
|
||||||
|
|
||||||
|
## The fix
|
||||||
|
|
||||||
|
Add a new flat-config block at the end of `eslint.config.mjs` (after
|
||||||
|
the existing `globalIgnores(...)` call, NOT inside the root rules
|
||||||
|
block) targeting only `scripts/**/*.js`:
|
||||||
|
|
||||||
|
```js
|
||||||
|
{
|
||||||
|
files: ['scripts/**/*.js'],
|
||||||
|
rules: {
|
||||||
|
'no-restricted-syntax': ['error', {
|
||||||
|
selector: 'CallExpression[callee.name="require"]',
|
||||||
|
message: 'Use ESM `import` syntax. `package.json` has "type": "module"; require() throws ReferenceError at runtime. See .convoys/fix-reset-db-script.md.',
|
||||||
|
}],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
```
|
||||||
|
|
||||||
|
The error message points at `.convoys/fix-reset-db-script.md` so that
|
||||||
|
the next agent / contributor who triggers the rule gets a 1-click path
|
||||||
|
to the exemplar fix (ESM top-level imports for `dotenv`, `neon`,
|
||||||
|
`bcrypt`) instead of having to re-derive it.
|
||||||
|
|
||||||
|
`scripts/migrations/**` is already in `globalIgnores` (from
|
||||||
|
`pick-a-name` Brief 2's migration script) and stays ignored — the
|
||||||
|
rule does not fire there even though the migration script is ESM and
|
||||||
|
correctly uses `import` (no need to re-lint files already excluded).
|
||||||
|
|
||||||
|
The rule fires on `CallExpression[callee.name="require"]` — the AST
|
||||||
|
shape of a plain `require('foo')` call. It does NOT fire on
|
||||||
|
`createRequire(import.meta.url)` patterns (which use `Module.createRequire`)
|
||||||
|
should one ever be needed; the AST callee is `createRequire`, not
|
||||||
|
`require`. If a future helper script legitimately needs CJS interop,
|
||||||
|
the right path is `await import('foo')` (ESM dynamic import) — the
|
||||||
|
rule will not block that either.
|
||||||
|
|
||||||
|
## Verification plan
|
||||||
|
|
||||||
|
1. `node --check eslint.config.mjs` → exit 0 (config parses).
|
||||||
|
2. `npm run lint` → exit 1 with **128 problems (81 errors, 47
|
||||||
|
warnings)** — verbatim match of the pre-convoy baseline (no
|
||||||
|
regression, no new false positives in the current tree).
|
||||||
|
3. **Negative test (apply, run, revert):** prepend
|
||||||
|
`const x = require('fs');` to `scripts/reset-db.js`, run
|
||||||
|
`npm run lint`, confirm exit 1 with the new rule firing at the
|
||||||
|
expected line/column and the documented message, then revert.
|
||||||
|
4. `npm run test:run` → 21/21 pass (no test surface touched; runs
|
||||||
|
only to confirm vitest is still green).
|
||||||
|
5. `rg "require\(" scripts/ --type js` → 0 hits (sanity check
|
||||||
|
confirming the current tree is clean and the rule has zero
|
||||||
|
positives to silence on day 1).
|
||||||
|
|
||||||
|
## Risks
|
||||||
|
|
||||||
|
- **False positives if anyone legitimately needs `require()` in
|
||||||
|
`scripts/**`.** None today (verified by step 5 — zero `require(`
|
||||||
|
hits across all helper scripts in the current tree after PR #25
|
||||||
|
and the `drop-public-setup` B2 sweep). If a future script
|
||||||
|
legitimately needs CJS interop (e.g. a dependency that only
|
||||||
|
exports CJS without an ESM wrapper), the fix is `await
|
||||||
|
import('foo')` — ESM dynamic import works in any ESM script and
|
||||||
|
is not flagged by the rule. If that's somehow not viable, the
|
||||||
|
escape hatch is a per-line `// eslint-disable-next-line
|
||||||
|
no-restricted-syntax` with a comment explaining why ESM doesn't
|
||||||
|
work; lint baseline tracking will catch the disable directive in
|
||||||
|
review.
|
||||||
|
- **Rule scope drift.** If someone adds a new top-level scripts
|
||||||
|
directory (`tools/`, `cli/`, etc.) the rule won't fire there. Low
|
||||||
|
risk — this repo has consolidated on `scripts/` since inception
|
||||||
|
and there's no signal of a second scripts directory being added.
|
||||||
|
Tracked here so the next refactor that reshapes the helper-script
|
||||||
|
layout knows to extend the `files:` glob.
|
||||||
|
- **ESLint v10 bump.** When `bump-eslint-10` lands (currently
|
||||||
|
upstream-blocked per Gotcha #10), re-verify this rule's selector
|
||||||
|
syntax against the v10 AST behavior. `no-restricted-syntax` is a
|
||||||
|
stable core rule going back to ESLint v1; no v10 deprecation is
|
||||||
|
expected, but the smoke check is cheap.
|
||||||
|
|
||||||
|
## Acceptance criteria
|
||||||
|
|
||||||
|
- `node --check eslint.config.mjs` exit 0
|
||||||
|
- `npm run lint` exit 1 with 128 problems (baseline preserved)
|
||||||
|
- `npm run test:run` 21/21 pass
|
||||||
|
- Negative test passes (rule fires on synthetic `require()` insertion,
|
||||||
|
reverts cleanly to 128 problems after the synthetic edit is
|
||||||
|
removed)
|
||||||
|
- Grep: 0 `require(` occurrences in `scripts/**/*.js` (current tree
|
||||||
|
is clean — rule starts with zero positives to silence)
|
||||||
|
|
||||||
|
## Out of scope
|
||||||
|
|
||||||
|
- Sweeping any other `scripts/**` file — current tree is clean
|
||||||
|
(verified by the grep step above). The rule is preventative,
|
||||||
|
not retroactive.
|
||||||
|
- Broadening the rule to all `.js` files at repo root — see
|
||||||
|
§ Design decision; `pages/api/**` is already correctly ESM and
|
||||||
|
the config files (`postcss.config.js`, `tailwind.config.js`,
|
||||||
|
`next.config.js`) intentionally use CJS-style exports that the
|
||||||
|
next-config base rules handle correctly.
|
||||||
|
- Bumping any deps (ESLint stays at v9 per Gotcha #10; no
|
||||||
|
`typescript-eslint` interaction since the rule is a core rule).
|
||||||
|
- The `purge-weak-creds-from-helpers` follow-up (the
|
||||||
|
`scripts/create-test-users.js` portion remains queued; this
|
||||||
|
convoy only adds the lint rule, not the weak-creds sweep).
|
||||||
|
|
||||||
|
## Owns
|
||||||
|
|
||||||
|
Parent (single-file ESLint config edit; no architect or implementer
|
||||||
|
subagent required — proven-pattern follow-up to PR #25).
|
||||||
|
|
||||||
|
## As-shipped
|
||||||
|
|
||||||
|
_(Stub — doc-writer to fill in post-merge with squash commit SHA,
|
||||||
|
PR URL, observed lint baseline before/after, CI gate results, and
|
||||||
|
any deviations from the planned shape.)_
|
||||||
|
|
@ -11,6 +11,15 @@ const eslintConfig = defineConfig([
|
||||||
'next-env.d.ts',
|
'next-env.d.ts',
|
||||||
'scripts/migrations/**',
|
'scripts/migrations/**',
|
||||||
]),
|
]),
|
||||||
|
{
|
||||||
|
files: ['scripts/**/*.js'],
|
||||||
|
rules: {
|
||||||
|
'no-restricted-syntax': ['error', {
|
||||||
|
selector: 'CallExpression[callee.name="require"]',
|
||||||
|
message: 'Use ESM `import` syntax. `package.json` has "type": "module"; require() throws ReferenceError at runtime. See .convoys/fix-reset-db-script.md.',
|
||||||
|
}],
|
||||||
|
},
|
||||||
|
},
|
||||||
]);
|
]);
|
||||||
|
|
||||||
export default eslintConfig;
|
export default eslintConfig;
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue