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

Migrate to eslint.config.js #15748

Merged
merged 2 commits into from Jul 12, 2023

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jul 4, 2023

Q                       A
License MIT

In this PR we migrate to the flat ESLint config eslint.config.js. Most transitions are done according to https://eslint.org/blog/2022/08/new-config-system-part-2/

  • The .eslintignore is now replaced by top level .ignores pattern as flat ESLint config does not support .eslintignore now. I removed unused patterns such as packages/*/dist
  • Migrating extends config with a files filter is a bit tricky. Currently I have to manually assign our config to the eslint-plugin-jest's recommended config, otherwise the eslint-plugin-jest:recommended will be enabled on source files if it is cascaded with top level config items.
    /cc @nzakas if I miss the proper approach

globals: {
// Flow
Iterator: true,
$Keys: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Flow globals are also removed as we are not using Flow anymore.

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 4, 2023

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/54836/

ignores: [
// @babel/register is the require() hook, so it will always be CJS-based
"packages/babel-register/**/*.{js,ts}",
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

excludedFiles is replaced by ignores because the former is not supported in flat config.
If I understand correctly, excludedFiles will exclude the matched file from this override config item but ESLint will continue matching it with other config items, while ignores will stop ESLint matching further config items to this file. @nzakas Is that correct?

Here it can be replaced because there are no more rules matching packages/babel-register/**/*.{js,ts}.

Copy link
Contributor

Choose a reason for hiding this comment

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

Almost. When both files and ignores are present in the same object, ignores acts like excludedFiles in that it simply filters out results that match files.

When ignores is present with any other key (i.e. rules) then it also acts like excludedFiles in that it matches every file except the ones matching patterns in ignores.

When ignores is present without any other keys in the same object then those files will always be ignored and never match any config objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! That is very helpful. I can't find much docs on ignores on the website, should I open a PR to update https://eslint.org/docs/latest/use/configure/ignore?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's on this page:
https://eslint.org/docs/latest/use/configure/configuration-files-new#specifying-files-and-ignores

Once we officially make flat config the default, we'll update the URL so this one is the default and the eslintrc one is marked as deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thank you! Now I might understand why searching "ignores" on the website does not return the options docs: most of them are rules options.

Are they supposed to show up once the flat config is made default?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. When we release v9.0.0, we will rearrange the documentation so that we favor everything related to flat config and the eslintrc docs will be made less prominent. So when you look up "ignores" it will show you docs related to flat config.

We're just in this weird intermediate state right now.

@JLHwung JLHwung force-pushed the migrate-to-eslint-config-js branch from b9b06b1 to 8c9cfd9 Compare July 5, 2023 13:11
@JLHwung JLHwung marked this pull request as ready for review July 5, 2023 13:11
"no-restricted-imports": ["error", { patterns: ["**/src/**"] }],
};
return config;
}),
Copy link
Contributor Author

@JLHwung JLHwung Jul 5, 2023

Choose a reason for hiding this comment

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

Previous config item:

{
      files: [
        ...testFiles,
        "packages/babel-helper-transform-fixture-test-runner/src/helpers.{ts,js}",
        "test/**/*.js",
      ],
      env: {
        jest: true,
      },
      extends: "plugin:jest/recommended",
      rules: {
        "jest/expect-expect": "off",
        "jest/no-identical-title": "off",
        "jest/no-standalone-expect": "off",
        "jest/no-test-callback": "off",
        "jest/valid-describe": "off",
        "import/extensions": ["error", "always"],
        "import/no-extraneous-dependencies": "off",
        "no-restricted-imports": ["error", { patterns: ["**/src/**"] }],
      },
    },

@JLHwung JLHwung added the PR: Internal 🏠 A type of pull request used for our changelog categories label Jul 5, 2023
Copy link
Contributor

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

This looks good! It can still be a bit tricky to use eslintrc-style configs (like the one in eslint-plugin-jest) and modify it to work differently, but fortunately, you have the full power of JavaScript available to help. Eventually, once plugins start exporting flat configs instead, this will get much easier.

@@ -1,4 +1,5 @@
{
"eslint.experimental.useFlatConfig": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heads up for contributors:
If you are using VSCode, please add this line to your project config after this PR gets merged, otherwise the linter extension will not work.

Copy link
Member

@liuxingbaoyu liuxingbaoyu left a comment

Choose a reason for hiding this comment

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

Amazing!

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thank you!

@nicolo-ribaudo nicolo-ribaudo merged commit ddbbaf6 into babel:main Jul 12, 2023
56 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the migrate-to-eslint-config-js branch July 12, 2023 09:09
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants