feat: Better conversion of .eslintrc.js files in migrate-config #172
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the migration of .eslintrc.js files to eslint.config.js by updating the CLI behavior to handle different JavaScript file types and enhancing the parsing of existing configuration code. Key changes include:
- Adjusting the CLI to differentiate handling based on file extensions (js, cjs, mjs).
- Updating tests and expected fixtures to validate these new behaviors.
- Modifying how configuration data is loaded and processed in the migration.
Reviewed Changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/migrate-config/tests/migrate-config-cli.test.js | Updates to test fixtures and migration path handling for various JS file types |
| packages/migrate-config/tests/fixtures/prisma/expected.mjs | Removal of the expected migrated code for this fixture |
| packages/migrate-config/tests/fixtures/prisma-js/expected.cjs | Adjustments in expected output for JS configurations with added globals and linterOptions |
| packages/migrate-config/tests/fixtures/prisma-js/.eslintrc.cjs | Minor configuration adjustments in the test fixture |
| packages/migrate-config/tests/fixtures/import-duplicate/expected.mjs | Expected file now empty to reflect duplicate import handling changes |
| packages/migrate-config/tests/fixtures/formidable-js/expected.cjs | New expected migrated configuration for formidable-js fixture |
| packages/migrate-config/tests/fixtures/formidable-js/.eslintrc.cjs | New configuration fixture for formidable-js |
| packages/migrate-config/tests/fixtures/backend-nx-skeleton-js/expected.cjs | New expected migrated configuration for backend nx skeleton (CommonJS) |
| packages/migrate-config/tests/fixtures/backend-nx-skeleton-js/.eslintrc.cjs | New configuration fixture for backend nx skeleton (CommonJS) |
| packages/migrate-config/tests/fixtures/backend-nx-skeleton-esm-js/expected.mjs | New expected migrated configuration for backend nx skeleton (ES module) |
| packages/migrate-config/tests/fixtures/backend-nx-skeleton-esm-js/.eslintrc.mjs | New configuration fixture for backend nx skeleton (ES module) |
| packages/migrate-config/src/migrate-config-cli.js | Updates the CLI to use the new migrateJSConfig for JavaScript file migration |
Files not reviewed (2)
- packages/migrate-config/package.json: Language not supported
- packages/migrate-config/tests/fixtures/formidable-js/LICENSE: Language not supported
| }, | ||
|
|
||
| extends: compat.extends( |
There was a problem hiding this comment.
The identifier 'compat' is used without being defined in this file. Consider importing or defining 'compat' (e.g., via 'const { FlatCompat } = require("@eslint/eslintrc");') to prevent reference errors.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Good bug found here. Need to see why compat is undefined.
4aec2aa to
fc597ef
Compare
| const isModule = !cjsExports; | ||
| const esmExport = isModule ? findDefaultExport(ast) : null; |
There was a problem hiding this comment.
I'm not sure if this is needed, since the eslintrc config system didn't support ESM config files.
There was a problem hiding this comment.
It didn't natively, I'm just not sure how folks were processing their config files before passing to ESLint, so I figured it's safer to check.
| // add ignore patterns from .eslintignore | ||
| if (ignorePatterns) { | ||
| migration.imports.get("eslint/config").bindings.push("globalIgnores"); | ||
| configArrayElements.push(createGlobalIgnores(ignorePatterns)); | ||
| } |
There was a problem hiding this comment.
If there's no .eslintignore, but only ignorePatterns in the config, this binding would be missing.
// .eslintrc.js
module.exports = {
ignorePatterns: ["foo"]
};// created eslint.config.js
const {
defineConfig, // missing `globalIgnores`
} = require("eslint/config");
module.exports = defineConfig([{}, globalIgnores(["**/foo"])]);There was a problem hiding this comment.
Also, patterns from .eslintignore should come after ignore patterns from the config.
Currently:
// .eslintrc.js
module.exports = {
ignorePatterns: ["foo"]
};# .eslintignore
bar
// created eslint.config.js
const {
defineConfig,
globalIgnores,
} = require("eslint/config");
module.exports = defineConfig([globalIgnores(["**/bar"]), {}, globalIgnores(["**/foo"])]);globalIgnores(["**/bar"]) should be after globalIgnores(["**/foo"]) in the array, or merged into a single element globalIgnores(["**/foo", "**/bar"]).
Prerequisites checklist
What is the purpose of this pull request?
Better support migration
.eslintrc.jsfiles toeslint.config.js.What changes did you make? (Give an overview)
.js,.mjs, and.cjsfilesRelated Issues
fixes #64
Is there anything you'd like reviewers to focus on?
I made some assumptions about JS config files in order to move forward, mostly assuming that certain keys (i.e.,
env) will be object literals rather than variable references or function calls. I also assumed that the file is exporting an object expression rather than a variable or a function call. A quick search of GitHub showed that the overwhelming majority of.eslintrc.jsfiles export an object expression.I'm outputting warnings when things are found that I don't know how to convert. This won't be perfect but it generally results in retaining more of the dynamic portions of a config file than the static approach.