Skip to content

Fix: check .js/.cjs/.mjs in format:check so the hook and CI agree - #733

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/prettier-scope-js-cjs-mjs
Open

Fix: check .js/.cjs/.mjs in format:check so the hook and CI agree#733
AmaadMartin wants to merge 3 commits into
mainfrom
fix/prettier-scope-js-cjs-mjs

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

No existing issue.

  1. Or, if no issue exists, describe the change:

Problem: The pre-commit hook formats .js with Prettier, but format:check in CI only checks .ts. Four tracked JS-family files have therefore drifted, and nothing catches them. A contributor who stages one of those files for an unrelated one-line edit gets a whole-file reformat mixed into the diff, with no warning.

Solution: I widened the format and format:check globs to **/*.{ts,js,cjs,mjs} and ran npm run format once. The four reformats are tool output; I hand-edited none of them. I also widened the lint-staged keys to the same four extensions, so the hook formats every file CI checks.

Why .cjs needs its own lint-staged key. .mjs joins the existing key, but .cjs cannot. That key runs eslint --fix before prettier --write, and ESLint has no CommonJS override today, so it errors on the two tracked .cjs fixtures:

$ npx eslint tests/integration/build_setup/ts_commonjs_native_addon/fake-onnxruntime-node/index.cjs \
             tests/integration/build_setup/ts_esm_native_addon/fake-onnxruntime-node/index.cjs
  4:10  error  A `require()` style import is forbidden  @typescript-eslint/no-require-imports
  4:10  error  A `require()` style import is forbidden  @typescript-eslint/no-require-imports
✖ 2 problems (2 errors, 0 warnings)

Adding cjs to that key would block any commit staging either fixture. So .cjs gets a Prettier-only key instead. Keeping the two globs disjoint also stops lint-staged from running eslint --fix and prettier --write on one file concurrently. Giving ESLint a **/*.cjs override with sourceType: 'commonjs' would let the keys merge, but that widens the ESLint scope and belongs in its own change.

.prettierignore gets api-reference/. Typedoc writes its out directory there (typedoc.json), and it is not in .gitignore. I measured this rather than assuming it: after npm run docs:generate, the widened glob flags five Typedoc asset bundles.

$ npm run docs:generate && npm run format:check
[warn] api-reference/typescript/assets/hierarchy.js
[warn] api-reference/typescript/assets/icons.js
[warn] api-reference/typescript/assets/main.js
[warn] api-reference/typescript/assets/navigation.js
[warn] api-reference/typescript/assets/search.js

CI is unaffected either way, because docs:check runs typedoc --emit none after format:check. The ignore entry protects a developer who runs npm run docs and then npm run format.

Two intended behaviour changes for contributors. A PR that leaves a .js, .cjs or .mjs file unformatted now fails CI. Staging a .mjs file now runs eslint --fix and prettier --write in the pre-commit hook, and staging a .cjs file runs prettier --write.

Overlapping open PRs (checked before writing code). Three open PRs touch the same two lines. None is merged, and main still has the narrow **/*.ts glob.

PR Base What it does
#552 #426, not main Same widening plus 3 of the 4 reformats, bundled with an ESM build-banner fix
#426 main Widens the ESLint scope, not the Prettier scope
#636 main Widens the same two lines to **/*.{ts,json,md}

This PR is the only one that sits on main and changes the Prettier scope alone. #552's format hunks cannot land until #426 lands. #636 covers JSON and Markdown, not JavaScript, so if it lands first the two widenings compose to **/*.{ts,js,cjs,mjs,json,md}.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

No new test file, deliberately. This change adds zero executable lines. The regression guard is npm run format:check in .github/workflows/validation.yaml. A unit test that read the glob string back out of package.json would be a tautology.

Mutation proof that the gate can fail. I reverted each of two reformatted files to its main copy and re-ran the gate. The old **/*.ts glob passes on the same dirty tree, which is the defect.

$ npm run format:check                                 # clean tree
All matched files use Prettier code style!             # exit 0

$ git checkout main -- .github/scripts/csat.cjs
$ npm run format:check
[warn] .github/scripts/csat.cjs
[warn] Code style issues found in the above file.      # exit 1
$ npx prettier "**/*.ts" --check                       # the OLD glob
All matched files use Prettier code style!             # exit 0  <- misses it

$ git checkout main -- .prettierrc.js
$ npm run format:check
[warn] .prettierrc.js
[warn] Code style issues found in the above file.      # exit 1
$ npx prettier "**/*.ts" --check                       # the OLD glob
All matched files use Prettier code style!             # exit 0  <- misses it

Integration suites that own the JS-family files this change could disturb, to show the reformats are inert:

$ npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts -t "js_"
Test Files  1 passed (1)   Tests  10 passed | 14 skipped (24)

$ npx vitest run --project integration tests/integration/skills/
Test Files  4 passed (4)   Tests  4 passed (4)

Manual End-to-End (E2E) Tests:

npm install && npm run build && npm run lint && npm run format:check   # all exit 0

# every tracked JS-family file is clean under the new glob
npx prettier --check $(git ls-files '*.js' '*.cjs' '*.mjs')
#   -> All matched files use Prettier code style!

# gitignored build output stays out of scope (Prettier honours .gitignore)
#   dist/ present after the build, plus a deliberately dirty coverage/probe.js
npm run format:check          # exit 0

# the hook now formats .mjs
#   edited console.log(  'x'  ) into scripts/auto-assignment.mjs, then:
git add scripts/auto-assignment.mjs && git commit -m "hook check"
#   -> "**/*.{js,mjs,ts} - 1 file"; the commit holds console.log('x');

# the hook now formats .cjs, closing the gap CI would otherwise catch
#   edited Object.values( ... ).sort() into .github/scripts/csat.cjs, then:
npm run format:check          # exit 1, [warn] .github/scripts/csat.cjs
git add .github/scripts/csat.cjs && git commit -m "hook check cjs"
#   -> "**/*.cjs - 1 file"; the commit holds Object.values(...).sort();
npm run format:check          # exit 0

# and the fixture ESLint rejects is still committable
#   edited exports.status = ( ) => '...-probe' into
#   tests/integration/build_setup/ts_commonjs_native_addon/fake-onnxruntime-node/index.cjs
git add ... && git commit -m "hook check fixture"
#   -> commit succeeds; the commit holds exports.status = () => '...-probe';

Prettier resolves to 3.8.4 from package-lock.json. npm run format rewrote exactly the four files listed in the diff and no .ts file.

Local validation on commit 231c1fd. The Actions queue on this fork is backlogged, so no check had started when I finished. I ran the CI steps locally on the exact pushed commit instead.

npm run build          # exit 0
npm run lint           # exit 0
npm run format:check   # exit 0
git status --porcelain # empty

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits August 6, 2026 13:10
Output of `npm run format` with the widened glob. No hand edits. Both
workflow entry points keep their export shape.
…ommit hook

The pre-commit hook formats .js with Prettier but CI only checked .ts, so
four tracked JS-family files drifted with nothing to catch them.

`cjs` stays out of the lint-staged key: that key runs `eslint --fix` first,
and ESLint has no CommonJS override, so it errors on the two tracked .cjs
fixtures and would block their commits.

Typedoc writes .js bundles to api-reference/, which is not gitignored, so
that directory is added to .prettierignore.
@AmaadMartin AmaadMartin closed this Aug 6, 2026
@AmaadMartin AmaadMartin reopened this Aug 6, 2026
The hook's Prettier scope now equals the format:check scope, so a .cjs
edit can no longer commit clean and then fail CI.

The key is separate from **/*.{js,mjs,ts} because that one runs
`eslint --fix` first, and ESLint has no CommonJS override: it errors on
the two tracked .cjs fixtures and would block their commits. Disjoint
globs also keep lint-staged from running eslint and prettier on one file
concurrently.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant