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.

Sign up for GitHub

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

Jump to bottom

fix: Avoid dirname for built-in configs. #15602

Closed
wants to merge 2 commits into from
Closed

fix: Avoid dirname for built-in configs. #15602

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce64343
fix: Avoid dirname for built-in configs.
zddai Feb 13, 2022
2dff129
fix: Avoid dirname for built-in configs.
zddai Feb 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions lib/cli-engine/cli-engine.js
View file
Open in desktop
Expand Up @@ -19,7 +19,8 @@ const fs = require("fs");
const path = require("path");
const defaultOptions = require("../../conf/default-cli-options");
const pkg = require("../../package.json");

const eslintRecommendedConfig = require("../../conf/eslint-recommended.js");
const eslintAllConfig = require("../../conf/eslint-all.js");
Copy link
Member

Choose a reason for hiding this comment

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

This would always load all core rules, which we try to avoid. Could we instead pass to new CascadingConfigArrayFactory() a function that loads these modules when called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing the PR!

May I ask why we try to avoid that? If it's for performance, is there a benchmark?
The point of the change is to avoid use of __dirname by resolving the configs before packaging (e.g. webpack) because __dirname harms usability of ESLint library and disallows webpacking.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's only for performance reasons. It's observable from the "Loading" test when you run npm run perf script.

However, this revealed that we have another place where eslint-all.js is unconditionally loaded, so if you'd like to see the difference in performance locally, you'd need to apply the change from #15606 first.

Copy link
Member

Choose a reason for hiding this comment

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

The point of the change is to avoid use of __dirname by resolving the configs before packaging (e.g. webpack) because __dirname harms usability of ESLint library and disallows webpacking.

Would webpack be able to include those two modules if they're require-d in a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried dynamic import and require but got errors in eslintrc/lib/flat-compat.js:

class FlatCompat {

    constructor({
        baseDirectory = process.cwd(),
        resolvePluginsRelativeTo = baseDirectory
    } = {}) {
        this.baseDirectory = baseDirectory;
        this.resolvePluginsRelativeTo = resolvePluginsRelativeTo;
        this[cafactory] = new ConfigArrayFactory({
            cwd: baseDirectory,
            resolvePluginsRelativeTo,
            eslintAllPath: path.resolve(dirname, "../conf/eslint-all.cjs"),
            eslintRecommendedPath: path.resolve(dirname, "../conf/eslint-recommended.cjs"),
+           eslintAllConfig: () => import("../conf/eslint-all.cjs"),    // 'import()' expressions are not supported yet. eslint[node/no-unsupported-features/es-syntax]
+           eslintRecommendedConfig: () => require("../conf/eslint-recommended.cjs")  // 'require' is not defined. eslint[no-undef]
        });
    }
//...

Hope someone can help me with the issues. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Since after this change those two configurations no longer need to be in files, we can inline their content directly in flat-compat.js and delete those .cjs files.

class FlatCompat {

    constructor({
        baseDirectory = process.cwd(),
        resolvePluginsRelativeTo = baseDirectory
    } = {}) {
        this.baseDirectory = baseDirectory;
        this.resolvePluginsRelativeTo = resolvePluginsRelativeTo;
        this[cafactory] = new ConfigArrayFactory({
            cwd: baseDirectory,
            resolvePluginsRelativeTo,
            getEslintAllConfig: () => ({
                settings: {
                    "eslint:all": true
                }
            }),
            getEslintRecommendedConfig: () => ({
                settings: {
                    "eslint:recommended": true
                }
            })
        });
    }

(I think it's better to name the new properties getEslintAllConfig and getEslintRecommendedConfig, to make clear that they're functions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

I've updated both PRs to address the comments.


const {
Legacy: {
Expand Down Expand Up @@ -616,8 +617,8 @@ class CLIEngine {
useEslintrc: options.useEslintrc,
builtInRules,
loadRules,
eslintRecommendedPath: path.resolve(__dirname, "../../conf/eslint-recommended.js"),
eslintAllPath: path.resolve(__dirname, "../../conf/eslint-all.js")
eslintRecommendedConfig,
eslintAllConfig
});
const fileEnumerator = new FileEnumerator({
configArrayFactory,
Expand Down
6 changes: 4 additions & 2 deletions lib/cli-engine/file-enumerator.js
View file
Open in desktop
Expand Up @@ -40,6 +40,8 @@ const getGlobParent = require("glob-parent");
const isGlob = require("is-glob");
const escapeRegExp = require("escape-string-regexp");
const { Minimatch } = require("minimatch");
const eslintRecommendedConfig = require("../../conf/eslint-recommended.js");
const eslintAllConfig = require("../../conf/eslint-all.js");

const {
Legacy: {
Expand Down Expand Up @@ -215,8 +217,8 @@ class FileEnumerator {
cwd = process.cwd(),
configArrayFactory = new CascadingConfigArrayFactory({
cwd,
eslintRecommendedPath: path.resolve(__dirname, "../../conf/eslint-recommended.js"),
eslintAllPath: path.resolve(__dirname, "../../conf/eslint-all.js")
eslintRecommendedConfig,
eslintAllConfig
}),
extensions = null,
globInputPaths = true,
Expand Down