Repository navigation
fix(cli): check only JS and TS files, gate on nothing checked, and summarize long runs - #180
Merged
Merged
Conversation
…mmarize long runs check parsed every file named outright as TSX, so the CSS, Markdown and JSON that `check $(git ls-files)` passes gave hundreds of syntax errors (209 for the demo's app/globals.css), and check_ui did the same for a path or filename such as globals.css or x.vue. check now skips such a file and counts it in the summary line; check_ui answers with an error. A JavaScript app whose pages are .js files had nothing checked by `check .`, which passed with --require-design-system. The gate now fails when the folders and globs given leave no file to check, and check names the .js files with JSX that folders left out, with the command that checks them. A run with 50 findings or more ends, in the default output, with the count per rule and the ten Tailwind palette classes found most, each with the token to use.
… .js pages when nothing was checked - A glob such as `app/**/*.css` or `**/*` no longer parses styles, docs and JSON as TSX: they are counted with the named ones as "files that are not JS or TS skipped". - When the only code left is `.js` pages with JSX, the "Nothing to check" line and the `--require-design-system` failure name them and the command that checks them, in place of the --include-design-system hint. The failure is one line, which is what the GitHub Action's annotation shows. - A folder of `.js` pages (`check app`) names them instead of "no files match", and still exits 2. - The JSX scan skips files it cannot read, `*.min.js` and files over 512 KB, and parses only files with an element where JSX can start one. - One SCRIPT_FILE in src/util/paths.ts for check and check_ui, without the made-up `.mtsx` and `.cjsx` extensions. - The changeset is a minor; action.yml and docs/required-check.md say how `paths` takes `. **/*.js` and when require-design-system fails.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Roadmap item: os-cli-summary (OS2-36 + OS2-48, wave 2).
What was wrong
Files that are not JS or TS were parsed as TSX.
checkandcheck_uiread every file they were given as TSX. In a git copy ofexamples/shadcn-demo,check $(git ls-files)on main reportedREADME.md241 errors,app/globals.css209,package.json26,tsconfig.json14 and everydocs/*.md48 to 86, allsyntax, and exited 1. Globs did the same:check "app/**/*.css"gave209 errors, 0 warnings in 1 file, andcheck "**/*"gave827 errors, 3 warnings in 11 of 13 files checked.check_uion the demo with{ path: "app/globals.css" }answeredok: falsewith the same 209 syntax errors. With{ code: "<template><button>x</button></template>", filename: "x.vue" }it answeredok: falsewith aprefer-design-system-componenterror, because it checked the Vue template as TSX.A JavaScript app had nothing checked, and the gate passed. Folders expand to
.tsxand.jsxfiles. In an app withcomponents/ui/button.jsxand its pages in.jsfiles (as Next.js's JavaScript template writesapp/page.js), main gave:A long run had no summary.
check .in Dub'sapps/web(corpus checkout, Dub 70c96f9) prints 14,228 lines on main, one per finding, ending with11962 errors, 245 warnings in 1009 of 1477 files checked.What changed
checkchecks only files that end in.tsx,.jsx,.ts,.js,.mts,.cts,.mjsor.cjs(any case), whether named outright or matched by a glob. The summary line counts the rest:(10 files that are not JS or TS skipped). In the demo copy,check "**/*"now gives7 errors, 3 warnings in 1 of 3 files checked (5 design-system files skipped; 10 files that are not JS or TS skipped), andcheck "app/**/*.css"givesNothing to check: 1 file that is not JS or TS skipped.(exit 0). If every file named outright is skipped, it printsNothing to check: ...and exits 0. Before, a list with no match exited 2 with "no files match".check_uireturns an error result for apathorfilenamethat is not JS or TS:check_ui checks .tsx, .jsx, .ts and .js files; app/globals.css is not one, so it was not checked.Thepathinput's description names the extensions. A path outside the root still gets the "outside the project root" error first. Both use oneSCRIPT_FILEfromsrc/util/paths.ts.--require-design-systemexits 2 when the folders and globs it is given leave no file to check: a JavaScript app whose pages are.jsfiles, a design-system package whose files are all its own components (check --require-design-system: nothing was checked (1 design-system file skipped). Pass --include-design-system to check it.), or a glob that matches only styles. If only files named outright were passed, it does not fail:check README.md app/globals.css(a pull request that changes only styles and docs) has no UI code to check, and a lint-staged run on a commit that only touchescomponents/uishould still pass.checkfinds the.jsfiles in them that contain JSX. It leaves out what git ignores, whatexcludecovers (tests by default), the design system's own files, anything a glob already names,*.min.js, files over 512 KB and files it cannot read. It names the files it found and the command that checks them. When that is all there is, the notice takes the place of the--include-design-systemhint, which would have checked the button and passed with the pages still unchecked:The gate failure is one line because the GitHub Action puts only the first stderr line in its error annotation. Run locally with
action/index.mjson that app, the annotation is now::error title=onsystem::onsystem check exited with 2: check --require-design-system: nothing was checked (1 design-system file skipped). 2 .js files have JSX ..., andpaths: . **/*.jschecks the pages (2 errors, 1 warning). Next to files that were checked, the notice goes to stdout in pretty output, as a::warningwith--format github, and to stderr with JSON or SARIF. It works per project at a monorepo root.(nearest token)when the rule says "Nearest token" rather than "Matches token", andno close tokenwhen the theme has none. JSON, SARIF and GitHub output do not change. On Dub'sapps/webthe end of the output is now:The ten pairs cover 5,634 of the 9,200 palette classes (61%). The output grows from 14,228 to 14,241 lines. Diffing the two outputs shows only the 13-line block above.
docs/ci.mdcovers the skipped files, the.jsnotice, the wider--require-design-systemgate and the summary. A test checks the summary example in the docs against real output.action.ymlanddocs/required-check.mdsay thatpathstakes globs without quotes (paths: . **/*.jsfor a JavaScript app) and thatrequire-design-systemalso fails whenpathsleaves nothing to check.--require-design-system, which the Action turns on by default, can now fail a job that passed before.The cost of the
.jsscanEvery
check <folder>reads the.jsfiles under the folder. A file is parsed only if it has</or/>and an element where only JSX can start one (after(,=>,=,returnand the like, with a closing tag or a/>after it). Over the.tsx,.jsxand.jsfiles of the corpus checkouts, 12,771 have an element by the TypeScript parse, and that check passes all 12,771. Timings, three runs each unless noted:check .over.jsfiles copied fromnode_modulesintopublic/js(96 MB; 135 have</, 48 are over 512 KB).jsfiles cut from those (71 MB), two runsapps/web, two runsOf the 352 library files under 512 KB, 122 have
</or/>, and 20 of those are still parsed (mostly React's own builds, whose comments and strings hold JSX examples). None is reported as having JSX.Choices that differ from the briefs
.jsfiles are named, not checked. The finding's first option was to add.jsto folder globs and check only the files that contain JSX. I chose its fallback (a notice, plus the gate) for three reasons. Checking them would give teams with a baseline new findings without a choice. The hook (plugins/onsystem/hooks/check-ui.mjs, which OS2-38 owns this wave) and the ESLint config (src/eslint/index.ts, outside this bundle) would still skip.js, so the surfaces would disagree. Andcorpus/snapshot.jsonmay change only once per wave, in X2-37.src/lint/rules, outside this bundle).init(OS2-38's file this wave) is not changed either.(nearest token)still comes from the message text. The summary looks for. Nearest tokenin the rule's message. A flag on the diagnostic would add a field to the JSON output andcheck_ui's results, and changesrc/lint/rules/hardcoded.ts, which other items edit this wave.test/check-summary.test.tsfails if the wording changes.How it was tested
New tests:
test/check-scope.test.ts(12 tests): named files that are not JS or TS (pretty, JSON, the gate with named files only, uppercase.SCSS); globs that match them (app/**/*.css, alone and with the gate,**/*, a file both named and globbed counted once);.ts,.js,.mjsand.cjsnamed files still checked; the JS app (the notice in place of the hint, the command it gives, exclude, tests and files without JSX left out, the JSON, GitHub and pretty channels); the gate in the JS app (one line) and in a design-system package; a folder of.jspages (check app, exit 2 with the notice, and a folder with no page still "no files match"); JSX after a comment, an arrow, a generic call in props and in a fragment found, and HTML in strings, a named regex group,*.min.jsand a file over 512 KB not counted; an unreadable.jsfile left out (skipped on Windows and as root); a pnpm monorepo root (includingcheck apps/web/appandcheck "**/*"); andcheck_uionapp/globals.css,x.vue,x.mtsx,x.cjsx, a.mtsfilename, a.tsxpath and a.jsfilename.test/check-summary.test.ts(3 tests): the summary lines and where they go, the top-10 cut and ordering (includingno close token), its absence from short runs and from JSON, SARIF and GitHub output, and thedocs/ci.mdexample.On main, 11 of the 12 scope tests fail; on the first commit of this PR, 8 do. The one that passes on main guards behaviour that must not change: named
.ts,.js,.mjsand.cjsfiles are still checked.Gate, all passing:
pnpm install --frozen-lockfile --prefer-offline,pnpm format:check,pnpm lint,pnpm typecheck,pnpm test(74 files passed, 1 skipped; 969 tests passed, 3 skipped),pnpm build(no change toschema.json),pnpm smoke(passed).Corpus:
pnpm corpuson this branch printsCounts match corpus/snapshot.json., and the blind sample fp-sample-20261016 still has 80 of its 80 labels. No count changes.Follow-ups
filesfor.jsfiles with JSX:plugins/onsystem/hooks/check-ui.mjs(OS2-38 this wave) andsrc/eslint/index.ts. They should move together with folder globs if.jspages are to be checked rather than named.check,inspectandinit(OS2-48, items 3 and 4), which needs the engine to count color classes that use tokens.docs/tools.md(thecheck_uirow) could mention the JS/TS-only error.src/detect.tskeeps its own script-file pattern (it also leaves out.d.ts); feat: read installed design-system packages that publish their sources #176 changes that file now.