fix: Avoid dirname for built-in configs.

[daidodo]

Load eslint:recommended and eslint:all configs via import instead file paths.

Fixes: #15575

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

• Documentation update
• Bug fix (template)
• New rule (template)
• Changes an existing rule (template)
• Add autofix to a rule
• Add a CLI option
• Add something to the core
• Other, please explain: Avoid using __dirname for built-in configs, i.e. eslint:recommended and eslint:all.

What changes did you make? (Give an overview)

I made changes to cli-engine.js and file-enumerator.js to pass in built-in config objects directly instead of file paths.

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

The main changes are in @eslint/eslintrc package (PR).
It should be merged and released first before changes here can be applied.

[linux-foundation-easycla]

• ❌ - zddai The commit (ce64343 ,2dff129566376aaf00c6b163b693d8b5dbde73db) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[daidodo]

The build failed because the main changes are in @eslint/eslintrc package (eslint/eslintrc#70).
It should be merged and released first before changes here can be applied.

[mdjermanovic]

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?

[daidodo]

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.

[mdjermanovic]

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.

[mdjermanovic]

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?

[daidodo]

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!

[mdjermanovic]

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).

[daidodo]

Thanks!

I've updated both PRs to address the comments.

[daidodo]

I've created new PRs (eslint/eslintrc#71 and #15616) to address the comments and with clean commit histories. So closing this PR.

Sorry for the inconvenience!