Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Use defineConfig() and globalIgnores() helpers #164

Merged
merged 2 commits into from
Mar 14, 2025

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Mar 13, 2025

Prerequisites checklist

What is the purpose of this pull request?

Update migrate-config to produce configs that use defineConfig() and globalIgnores()

What changes did you make? (Give an overview)

  • Updated the return type of createGlobalIgnores function to CallExpression and modified its implementation to use globalIgnores instead of an object expression. (packages/migrate-config/src/migrate-config.js) [1] [2]
  • Modified migrateConfigObject to push properties directly instead of mapping them into an object. (packages/migrate-config/src/migrate-config.js)
  • Ensured defineConfig is always used by adding it to the imports and creating a defineConfigNode for the configuration array. (packages/migrate-config/src/migrate-config.js) [1] [2] [3] [4]
  • Updated all test fixtures to reflect the new configuration format using defineConfig and globalIgnores. This includes changes in both CommonJS (.cjs) and ES Module (.mjs) formats. (packages/migrate-config/tests/fixtures/basic-eslintrc/expected.cjs, packages/migrate-config/tests/fixtures/basic-eslintrc/expected.mjs, packages/migrate-config/tests/fixtures/gitignore-complex/expected.cjs, packages/migrate-config/tests/fixtures/gitignore-complex/expected.mjs, packages/migrate-config/tests/fixtures/gitignore-simple/expected.cjs, packages/migrate-config/tests/fixtures/gitignore-simple/expected.mjs, packages/migrate-config/tests/fixtures/import-duplicate/expected.cjs, packages/migrate-config/tests/fixtures/import-duplicate/expected.mjs, packages/migrate-config/tests/fixtures/no-globals-for-env/expected.cjs, packages/migrate-config/tests/fixtures/no-globals-for-env/expected.mjs, packages/migrate-config/tests/fixtures/overrides-extends/expected.cjs) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20] [21] [22] [23] [24] [25] [26] [27]

Related Issues

fixes #163

Is there anything you'd like reviewers to focus on?

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the created config doesn't have imports other than eslint/config, migration still prints out that some packages should be added (but doesn't list any):

Migrating .eslintrc.json

Wrote new config to ./eslint.config.mjs

You will need to install the following packages to use the new config:


You can install them using the following command:

npm install  -D

I think the problem is that this code doesn't check whether there are any imports left after filtering out those that don't require additional installation:

if (result.imports.size) {
const addedImports = [...result.imports.entries()]
.filter(([key, imp]) => imp.added && !key.startsWith("node:"))
.map(([key]) => key);
console.log(
"\nYou will need to install the following packages to use the new config:",
);
console.log(`${addedImports.map(imp => `- ${imp}`).join("\n")}\n`);
console.log("You can install them using the following command:\n");
console.log(`npm install ${addedImports.join(" ")} -D\n`);
}

@nzakas
Copy link
Member Author

nzakas commented Mar 14, 2025

Fixed

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 727ec5d into main Mar 14, 2025
18 checks passed
@mdjermanovic mdjermanovic deleted the define-config branch March 14, 2025 16:03
@github-actions github-actions bot mentioned this pull request Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Change Request: Update migrate-config to use defineConfig
2 participants