Conversation
Step 1 of the major-version migration plan. Upgrades `eslint` 9.39 → 10.2 and `typescript` 5.9 → 6.0; shipping these before Astro 6 means `astro check`/`tsc` are already running on the new compiler when we chase down Astro-6 API breaks — one signal at a time instead of two. The only blocker was `eslint-plugin-jsx-a11y@6.10.2` (latest), whose peer range caps at `eslint ^9`; the two open ESLint-10 PRs upstream (#1081, #1079) are still unmerged. Drop the plugin per the migration plan's explicit guidance. The `astro.configs["jsx-a11y-strict"].rules` spread in eslint.config.js was already a silent no-op (the getter returns a `Linter.Config[]` array, and `...(array).rules` is `...undefined`), so removing the plugin costs zero actual lint coverage. Reintroducing real a11y linting for .astro files needs a separate pass that consumes the array-shaped preset correctly. `@typescript-eslint/*@8.58.2` already peers on `eslint ^10` and `typescript <6.1`, so no bump needed there. `@astrojs/check@0.9.8` still peers on `typescript ^5.0.0`; the override warning is cosmetic since it's a CLI wrapper around `@astrojs/language-server` and doesn't gate on the peer version — `astro check` passes clean against TS 6 locally. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 636144e81e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "astro-seo": "^0.8.4", | ||
| "clsx": "^2.1.1", | ||
| "eslint": "^9.39.4", | ||
| "eslint": "^10.2.1", |
There was a problem hiding this comment.
Add @eslint/js to dependencies
After bumping to eslint@10.2.1, @eslint/js is no longer installed transitively (it does not appear anywhere in the new lockfile), but eslint.config.js still imports it on line 1. On a clean npm ci environment this causes npm run lint to fail at startup with a module resolution error for @eslint/js, so CI and fresh local setups cannot run lint until @eslint/js is declared directly.
Useful? React with 👍 / 👎.
Keep cspell v10 from main. Skip re-adding eslint-plugin-jsx-a11y — its latest release (6.10.2) caps the eslint peer at ^9, incompatible with the eslint 10 bump on this branch. The previous jsx-a11y rules spread in eslint.config.js was already a silent no-op, so lint coverage is unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PR Validation ❌Some checks failed. Please review the errors above. Checks Performed:Please check the workflow logs for details on which checks failed. This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
Code Review: Bump to eslint 10 + typescript 6OverviewClean Step 1 execution of the migration plan — surgically correct scope (lint/TS track only, Astro/Tailwind untouched). The bulk of the diff is lock file churn from ESLint's own dependency tree modernizing; the actual source change is a single-line deletion in Strengths
Issues / Risks1. Node 20 pinning in CI (moderate risk — unchecked test plan item)ESLint 10 declares Recommendation: Verify the 2. Accessibility linting gap needs a tracking issueThe site's CLAUDE.md calls out "reasonable accessibility support" as a goal. Even though the Recommendation: File a follow-up issue or add a bullet to the migration plan noting that 3. Minor:
|
CI `npm ci` fails on the @astrojs/check@0.9.8 peer dep pinning to typescript ^5.0.0. The TS 6 bump commit (636144e) flagged this as a cosmetic override warning, but strict `npm ci` treats it as an error. `legacy-peer-deps=true` matches the lockfile's resolution and unblocks CI until @astrojs/check ships a TS 6-compatible release. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PR Validation ❌Some checks failed. Please review the errors above. Checks Performed:Please check the workflow logs for details on which checks failed. This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
After the eslint 10 bump, @eslint/js was no longer pulled in transitively, but eslint.config.js still imports it. Declare it directly so lint runs on clean npm ci environments. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PR Validation ✅All checks passed! Ready for review. Checks Performed:✓ Linting This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
Code Review — PR #41: Bump to eslint 10 + typescript 6Overall: Clean, well-scoped migration step with good rationale documented in Issues1. The PR adds The most likely trigger is
Either way, the PR description should document why this file was added. 2. CI node version is misreported in the PR description (low) The description's unchecked CI item says "ubuntu-latest + Node 20", but 3. No tracking for jsx-a11y re-introduction (low) CLAUDE.md calls out "reasonable accessibility support" as a site goal. The PR correctly notes that the What looks good
Minor note (not a blocker)
|
PR Validation ✅All checks passed! Ready for review. Checks Performed:✓ Linting This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
Code ReviewThis is a clean, well-scoped dependency bump. The PR description is unusually thorough (the explanation of why the Node version constraint — potential CI breakESLint 10's engine requirement jumped from
|
…ride The .npmrc `legacy-peer-deps=true` was a blunt mitigation for a single known conflict: @astrojs/check@0.9.8 pins its typescript peer to ^5.0.0 but we're on TS 6. Applying it globally suppresses peer validation for every future install — hiding genuine conflicts we'd want to see. Narrow the scope via package.json `overrides` so only @astrojs/check's typescript peer is rewritten to the root version. Every other peer-dep conflict (current or future) still fails fast under `npm ci`. When @astrojs/check ships a TS 6-compatible release, deleting this entry is a one-liner. Verified: `rm -rf node_modules package-lock.json && npm install`, `rm -rf node_modules && npm ci`, `npm run build`, `npm run lint` all pass. `npm ls @astrojs/check typescript` confirms @astrojs/check@0.9.8 resolves typescript@6.0.3 (deduped from root). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PR Validation ✅All checks passed! Ready for review. Checks Performed:✓ Linting This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
Code Review — PR #41: Bump to eslint 10 + typescript 6Status: CI is green (all checks passing as of the latest validation run), so this is a merge-readiness review. What's in this PR (beyond the obvious version bumps)The diff contains more than the title suggests — worth being explicit:
Things that look correct
Type isolation is well-structured. Pinning Astro's nested
The One open item before mergingAccessibility lint gap needs a tracking issue. Prior reviews noted this, and it remains unaddressed in the PR. CLAUDE.md lists "reasonable accessibility support" as an explicit goal. Even though zero a11y rules were enforced before (no-op spread), removing the plugin without a concrete follow-up issue creates drift risk across the remaining migration steps. This is especially salient because the blocking upstream PRs ( Suggested action: Add a bullet to Minor (non-blocking)
Overall: Mechanically correct, well-tested (CI green, thorough test plan). The one ask is capturing the a11y lint restoration as a tracked item so it survives across the remaining migration steps. |
Establish an explicit Node floor that clears the ESLint 10 compatibility gap in the 20.x series. Adds engines to package.json, .nvmrc for local version managers, engine-strict=true in .npmrc for hard failures on mismatched Node, and bumps the workflow's setup-node range to match. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PR Validation ✅All checks passed! Ready for review. Checks Performed:✓ Linting This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
Code ReviewOverviewThis PR bumps ESLint 9→10 and TypeScript 5→6 as part of a staged major-version migration. The changes are clean and the PR description is detailed. A few items worth flagging below. Correctness
Minor Issues
AccessibilityThe PR correctly identifies that
so this doesn't silently fall through the cracks. Positive Notes
SummaryThe logic is sound and the test plan is thorough. The main actionable items are: align |
Summary
Step 1 of the major-version migration plan (
planning/major-migration-plan.md): bumpeslint9.39 → 10.2 andtypescript5.9 → 6.0, shipped ahead of Astro 6 soastro check/tscare already on the new compiler when we hit Astro-6 API breaks.The only blocker was
eslint-plugin-jsx-a11y@6.10.2(peer caps ateslint ^9, and upstream PRs #1081/#1079 for ESLint-10 support remain unmerged), so the plugin is dropped per the migration plan's guidance. Theastro.configs["jsx-a11y-strict"].rulesspread ineslint.config.jswas already a silent no-op (the getter returnsLinter.Config[], so.ruleswasundefined), so removing it costs zero actual lint coverage — a correct reintroduction rides in a later pass.@typescript-eslint/*@8.58.2already peers oneslint ^10+typescript <6.1so stays put.@astrojs/check@0.9.8'stypescript: ^5.0.0peer warning is cosmetic (it's a CLI wrapper around@astrojs/language-server, no runtime gate) andastro checkruns clean against TS 6.Test plan
npm run lintpassesnpm run build(=astro check && astro build) passes — TS 6 smoke test; 24 pages generatednpm run validate:allpasses (lint + spellcheck + build + HTML spellcheck + link validation)git grep jsx-a11youtsideplanning//package-lock.jsonreturns zero hitsbuild.ymlon ubuntu-latest + Node 20) reproduces the local green🤖 Generated with Claude Code