Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Node-based codemod intended to modernize legacy Karma/Mocha test files under test/ by performing automated AST-driven refactors.
Changes:
- Adds a new CLI codemod (
build/codemods/modernize-test-files.mjs) to transform test sources (arrow conversions,var->let/const, string concat->template literals, remove'use strict'). - Wires the codemod into
package.jsonscripts (codemod:modernize-tests). - Introduces
recastas a dev dependency to preserve formatting while printing transformed files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| package.json | Adds a codemod npm script and adds recast to support AST printing. |
| build/codemods/modernize-test-files.mjs | New codemod CLI implementing multiple source-to-source transformations for test files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Leading space inside opening backtick (Prettier-friendly padding). */ | ||
| let buffer = ' '; | ||
|
|
||
| for (const part of parts) { | ||
| if (t.isStringLiteral(part)) { | ||
| buffer += escapeTemplateQuasiText(part.value); | ||
| continue; | ||
| } | ||
| const raw = buffer; | ||
| buffer = ''; | ||
| quasis.push(t.templateElement({ raw, cooked: raw }, false)); | ||
| expressions.push(part); | ||
| } | ||
| /** Trailing space inside closing backtick. */ | ||
| buffer += ' '; | ||
| quasis.push(t.templateElement({ raw: buffer, cooked: buffer }, true)); |
There was a problem hiding this comment.
partsToTemplateLiteral unconditionally injects a leading and trailing space inside the generated template literal (let buffer = ' ' and buffer += ' '), which changes runtime string values (e.g., "a" + b becomes a template that yields " a${b} "). A codemod that is meant to be behavior-preserving should not add extra characters; consider removing the padding or only applying it when the original concatenation already begins/ends with whitespace and/or when the chain is clearly multiline HTML.
| /** | ||
| * Whether converting this `var` to `let`/`const` preserves behavior. `var` in nested | ||
| * blocks hoists to the function; `let`/`const` does not — only convert when the | ||
| * declaration sits in the direct body of a function or method (not `if`/`for`/`try`, | ||
| * not `for (var ...)`, not top-level script `var`). | ||
| * | ||
| * @param {import('@babel/traverse').NodePath<import('@babel/types').VariableDeclaration>} varPath | ||
| */ | ||
| function canSafelyConvertVarDeclaration(varPath) { | ||
| const parent = varPath.parentPath; | ||
| if (!parent) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( | ||
| parent.isForStatement() || | ||
| parent.isForOfStatement() || | ||
| parent.isForInStatement() | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!parent.isBlockStatement()) { | ||
| return false; | ||
| } | ||
|
|
||
| const block = parent; | ||
| const container = block.parentPath; | ||
| if (!container) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( | ||
| container.isFunctionDeclaration() || | ||
| container.isFunctionExpression() || | ||
| container.isArrowFunctionExpression() | ||
| ) { | ||
| return container.get('body') === block; | ||
| } | ||
|
|
||
| if (container.isClassMethod() || container.isObjectMethod()) { | ||
| return container.get('body') === block; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
The var -> let/const transform only checks that the declaration is in a function/method body, but it doesn't account for var hoisting vs. let/const TDZ. If any binding is referenced before the declaration, converting will change behavior (from undefined to ReferenceError). This needs an additional safety check (e.g., ensure all reference locations occur after the declaration, or restrict conversion to declarations that are already at the top of the function body).
| /** | ||
| * @param {string} source | ||
| * @param {string} filename | ||
| */ | ||
| /** | ||
| * Parse with recast so `recast.print` can reuse original formatting (including blank | ||
| * lines). `tokens: true` helps recast reprint accurately. | ||
| * | ||
| * @param {string} source | ||
| * @param {string} filename | ||
| */ |
There was a problem hiding this comment.
There is an extra/empty JSDoc block immediately before the real parseSource docstring (the @param {string} source / @param {string} filename block at lines 380-383). It looks like a leftover and can confuse generated docs and readers; remove the stray block so only the full parseSource comment remains.
| "outdent": "^0.8.0", | ||
| "patch-package": "^8.0.0", | ||
| "prettier": "^3.0.3", | ||
| "recast": "^0.23.11", | ||
| "revalidator": "^0.3.1", |
There was a problem hiding this comment.
build/codemods/modernize-test-files.mjs imports @babel/parser, @babel/traverse, and @babel/types, but those packages are not listed in devDependencies (currently only @babel/core/plugins are). Relying on transitive dependencies is brittle; add these as explicit devDependencies so the codemod keeps working if Babel’s internal dependency graph changes.
| } else if (a === '--prettier') { | ||
| args.prettier = true; | ||
| } else if (!a.startsWith('-')) { | ||
| args.globs.push(a); |
There was a problem hiding this comment.
parseArgs silently ignores unknown flags (any arg starting with - that isn't recognized). For a CLI codemod, this makes typos hard to notice; consider treating unknown options as an error (print help + set non-zero exit code) to avoid accidentally running with unintended defaults.
| args.globs.push(a); | |
| args.globs.push(a); | |
| } else { | |
| console.error(`Unknown option: ${a}`); | |
| printHelp(); | |
| process.exit(1); |
| "outdent": "^0.8.0", | ||
| "patch-package": "^8.0.0", | ||
| "prettier": "^3.0.3", | ||
| "recast": "^0.23.11", | ||
| "revalidator": "^0.3.1", |
There was a problem hiding this comment.
recast was added to devDependencies, but package-lock.json is not updated in this PR. If CI or contributors use npm ci (which this repo does in release tooling), the lockfile needs to be regenerated/committed to keep installs reproducible.
We need to clean up that old test dir. Figured I'd have Cursor generate a code mod for us to do it.