Upgrade to ESLint 10 and @cloudfour/eslint-config v26.1.0 - #2467
Merged
Conversation
This does not need TypeScript 6. xo peers `typescript: ">=6"`, but that peer is optional: on 5.9.3 the install resolves, `npm ci` works, and `typescript-estree` resolves our root TypeScript rather than the copy npm records for xo. That matters, because TypeScript 6 makes the type-aware rules report nonsense here -- `while (scrollHeight > clientHeight)` as "always falsy" -- and their autofix would delete the guards it misreads. See #2465, which stays parked. 405 findings became zero. Most of them went through two scoped rules rather than edits to source: - `camelcase` (226) is off where the snake_case names are someone else's: Twig template variables in stories and arg files, design token keys that become CSS custom properties, mock data shaped like WordPress's `paginate_links()`, and terser options. - `jsdoc/require-description` (30) is off for .js files. With `checkJs` on, JSDoc is how those files carry types, and requiring prose above every `/** @PARAM {Args} args */` produces filler. Also renames `import/order` to `import-x/order`, which v26 documents. Three things needed care rather than the obvious fix: - `non-nullable-type-assertion-style` autofixed `closest('.js-sky-nav') as HTMLElement` into `closest('.js-sky-nav')!`, which drops the narrowing -- `!` yields `Element`, and `offsetWidth` is on `HTMLElement`. `tsc` went from clean to one error. The generic form satisfies the rule and keeps the type. - `prefer-number-coercion` wanted `Number()` instead of `Number.parseFloat()` on `tokens.time.transition.slow.value`. That value is "0.4s", so `Number()` gives NaN and the transition becomes `transform NaNs`. Left as `parseFloat` with the reason recorded. - `require-unicode-regexp` is off in browser tests. Regexes passed to Vitest locators are serialised into a selector string rather than executed, and its parser rejects the `v` flag outright, which turned two passing tests into failures. The rest are ordinary: the `v` flag elsewhere, unused capture groups made non-capturing, named default exports, `node:process` imports, leading slashes dropped from `files` patterns, four Twig imports renamed to match the 114 that already use camelCase, and declarations moved below early exits. `terser` is now a declared devDependency rather than one we relied on arriving through the rollup plugin. `sideEffects` and `require-types-in-exports` are off with a note. Both describe what we publish, and both break consumers quietly when wrong, so they deserve a deliberate look at packaging rather than a change here. The lockfile was updated by removing the old config and installing the new one. Regenerating it from scratch instead changed 226 package versions including a major vitest bump and a `@types/node` downgrade; this way it is 36 changes, 13 major, all in the ESLint ecosystem. Verified: lint, `tsc`, build, and 25/25 tests. Closes #2458
|
✅ Deploy Preview for cloudfour-patterns ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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
1 participant
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.
Overview
Supersedes #2458, which Renovate opened for v26.0.0 and which could not install. With #2466 merged the
eslint-plugin-reactblocker is gone, and this takes us the rest of the way: ESLint 10.9.1 and@cloudfour/eslint-configv26.1.0.This does not need TypeScript 6, so #2465 can stay parked. xo declares
typescript: ">=6", but that peer is optional. On 5.9.3 the install resolves,npm ciworks, andtypescript-estreeresolves our root TypeScript rather than the copy npm records for xo. That matters more than it sounds: on TypeScript 6 the type-aware rules here report nonsense —while (scrollHeight > clientHeight)comes back as "always falsy" — andno-unnecessary-conditionis autofixable, sonpm run lint:jswould delete the guards it misreads. The details are in #2465.v26 reports 405 findings on this repo. Most were not really 405 problems. Two scoped rules account for 256 of them, and neither is a shrug:
camelcase(226) — every snake_case identifier involved belongs to someone else's naming scheme. Twig template variables in stories and arg files are the template's contract; design token keys become CSS custom properties and Sass variables in our published output; demo data mirrors WordPress'spaginate_links(); and one is a terser option. Renaming any of them breaks something.jsdoc/require-description(30) —checkJsis on, so JSDoc is how our.jsfiles carry types. Demanding a prose description above every/** @param {Args} args */produces filler, not documentation.The remaining ~150 are ordinary fixes: the
vflag on regexes, unused capture groups made non-capturing, named default exports,node:processimports, declarations moved below early exits, and leading slashes dropped fromfilespatterns. Four Twig imports were renamed to camelCase to match the 114 that already are.terserbecomes a declared devDependency instead of one we relied on reaching us through the rollup plugin.Three that would have been wrong to just fix
Worth flagging, because in each case the rule's suggested change breaks something:
non-nullable-type-assertion-styleautofixedclosest('.js-sky-nav') as HTMLElementintoclosest('.js-sky-nav')!. Those are not equivalent —!only removes the null and leavesElement, whileoffsetWidthlives onHTMLElement.tscwent from clean to one error. The generic form (closest<HTMLElement>(…)!) satisfies the rule and keeps the type.prefer-number-coercionwantedNumber()in place ofNumber.parseFloat()ontokens.time.transition.slow.value. That value is"0.4s", soNumber()yieldsNaNand the sky nav's transition becomestransform NaNs. Left asparseFloat, with the reason in a comment.require-unicode-regexpis off in browser tests. Regexes handed to Vitest locators are serialised into a selector string rather than executed, and its parser rejects the flag outright —Error while parsing selector button[name=/^reply$/iv]. Adding it turned two passing tests into failures.package-json/prefer-side-effects-fieldandrequire-types-in-exportsare off with a note rather than acted on. Both describe what we publish and both break consumers quietly when wrong — an inaccuratesideEffectslets bundlers drop our styles — so they deserve a deliberate look at packaging rather than a change smuggled into a lint upgrade.Lockfile
Updated by removing the old config and installing the new one, rather than regenerating. Regenerating from scratch changed 226 package versions, 56 of them major, including a
@vitest/*3→4 bump and a@types/nodedowngrade from 26 to 22. This way it is 36 changes, 13 major, every one in the ESLint ecosystem.Screenshots
Testing
npm ci, thennpm run preprocess, thennpm run lint:check— passes with no outputnpm run typeandnpm run build— both succeednpm test— 12 test files, 25 tests, all passing. The two browser tests incommentandsky-navare the ones the regex flag broke, so they are the ones worth seeing greennpm startand open the Sky Nav story. Toggle the menu open and closed at a narrow viewport — it should slide smoothly rather than snapping, which is what aNaNtransition duration would look likeCloses #2458
Related: #2465 (parked — TypeScript 6 is not needed for this)