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: support eslint.config.js #95

Merged
merged 13 commits into from May 15, 2023
Merged

feat: support eslint.config.js #95

merged 13 commits into from May 15, 2023

Conversation

aladdin-add
Copy link

@aladdin-add aladdin-add commented Apr 21, 2023

fixes #89

an example:

const nodeRecommended = require("eslint-plugin-n/configs/recommended")
module.exports = [
    nodeRecommended,
    {
        rules: {
            "n/exports-style": ["error", "module.exports"]
        }
    }
]

@aladdin-add aladdin-add force-pushed the feat/new-config branch 2 times, most recently from 4f8086b to 3830d3e Compare April 21, 2023 11:00
@aladdin-add aladdin-add marked this pull request as draft April 21, 2023 12:02
@aladdin-add aladdin-add marked this pull request as ready for review April 21, 2023 12:21
Copy link
Member

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

Isn't this feature still very much a work in progress in ESLint core?

The official docs (https://eslint.org/docs/latest/use/configure/configuration-files-new) say:

This config system is feature complete but not enabled by default. To opt-in, place an eslint.config.js file in the root of your project or set the ESLINT_USE_FLAT_CONFIG environment variable to true.

What are the implications of this change?

@voxpelli
Copy link
Member

Noticed EvgenyOrekhov/eslint-config-hardcore#726 when I scrolled further in my notifications and now I think I have missed an announcement on the ESLint blog or something 🙈

@voxpelli
Copy link
Member

And now I noticed #89 as well

Would have been great to have a link to that in the description, and if done using the "fixes #89" wording then that issue would be automatically closed when this PR is merged 🙂

@aladdin-add
Copy link
Author

@voxpelli I've mentioned the issue in the commit: 3830d3e, but forgot to add it in the PR desc. 😂

updated.

@mdjermanovic
Copy link
Member

Thanks for working on this! Yes, since we've finished Phase 2 (eslint/eslint#13481) it's time for plugins to start adding support for the new config system, and in Phase 3 we have this task for this particular plugin.

@mdjermanovic
Copy link
Member

It would be nice to add metadata:

https://eslint.org/docs/latest/extend/plugins#metadata-in-plugins

@aladdin-add
Copy link
Author

👍 pushed a commit: 33045c6. 😏

eslint.config.js Outdated Show resolved Hide resolved
aladdin-add added a commit that referenced this pull request Apr 29, 2023
aladdin-add added a commit that referenced this pull request Apr 29, 2023
@aladdin-add aladdin-add requested review from mdjermanovic and a team April 29, 2023 17:36
@aladdin-add aladdin-add mentioned this pull request May 10, 2023
@aladdin-add
Copy link
Author

ptla :). cc @mdjermanovic


module.exports = {
plugins: { n: mod },
languageOptions: { globals: mod.configs.recommended.globals },
Copy link
Member

Choose a reason for hiding this comment

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

Missing sourceType?

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually not sure if there should be recommended config because it would be wrong for .cjs/.mjs files. The current eslintrc config has overrides for them, so it works well. We could do something similar here - export an array of configs with the files key - but that should generally be avoided in the new config system because it makes the config more difficult to manipulate with (e.g., to apply it to a specific directory only).

For commonjs projects, user's config should typically be:

// user's eslint.config.js

const nodeRecommendedScript = require("eslint-plugin-n/configs/recommended-script");
const nodeRecommendedModule = require("eslint-plugin-n/configs/recommended-module");

module.exports = [
    {
        files: ["**/*.js", "**/*.cjs"],
        ...nodeRecommendedScript
    },
    {
        files: ["**/*.mjs"],
        ...nodeRecommendedModule
    }
]

For "type": "module" projects:

// user's eslint.config.js

import nodeRecommendedScript from "eslint-plugin-n/configs/recommended-script.js";
import nodeRecommendedModule from "eslint-plugin-n/configs/recommended-module.js";

export default [
    {
        files: ["**/*.js", "**/*.mjs"],
        ...nodeRecommendedModule
    },
    {
        files: ["**/*.cjs"],
        ...nodeRecommendedScript
    }
];

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

In the beginning, to minimize the difference with the eslintrc presets, I had wondered if a configuration could be provided:

module.exports = packageJson.type === "module" ? moduleConfig : scriptConfig;

but it won't work well if it's using with ".cjs" and ".mjs". /_ \

Yes, this way sounds much clearer. Will remove it and document the usage. Thanks for the thoughtful feedback! ❤️

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! I left only some minor notes.

@@ -56,7 +59,7 @@
"lint": "npm-run-all \"lint:*\"",
"lint:docs": "markdownlint \"**/*.md\"",
"lint:eslint-docs": "npm run update:eslint-docs -- --check",
"lint:js": "eslint lib scripts tests/lib .eslintrc.js",
"lint:js": "eslint .",
Copy link
Member

Choose a reason for hiding this comment

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

Compared to the previous version, this will now lint test/fixtures too, so just to check if that's intentional?

js.configs.recommended,
nodeRecommended,
...compat.extends("plugin:eslint-plugin/recommended", "prettier"),
]
Copy link
Member

Choose a reason for hiding this comment

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

Compared to the previous (.eslintrc.js) config, this one might be missing this rule:

rules: {
"eslint-plugin/require-meta-docs-description": "error",
},

}
}
]
```
Copy link
Member

Choose a reason for hiding this comment

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

Maybe there should also be an example with configs/recommended-module, for ESM ("type": "module") projects?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: support the new eslint config.
3 participants