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

Bug: FlatCompat.config doesn't work with overrides #125

Closed
Ayc0 opened this issue Aug 2, 2023 · 7 comments · Fixed by #126 or #124
Closed

Bug: FlatCompat.config doesn't work with overrides #125

Ayc0 opened this issue Aug 2, 2023 · 7 comments · Fixed by #126 or #124
Assignees
Labels
accepted bug Something isn't working

Comments

@Ayc0
Copy link

Ayc0 commented Aug 2, 2023

Step to reproduce

  • eslint: 8.46.0
  • @eslint/eslintrc: 2.1.1

You can open & run: https://runkit.com/ayc0/64cac9cf2e21ef0008bee16d

This runkit gets lost / deleted / edited, here is the raw code
const code = 'console.log(process.pwd())';

const run = async (ESLint, config, filePath) => {
    const eslint = new ESLint(config);
    
    // 2. Lint text.
    const results = await eslint.lintText(code, { filePath });

    // 3. Format the results.
    const formatter = await eslint.loadFormatter("stylish");
    const resultText = formatter.format(results);

    // 4. Output it.
    console.log(resultText);
}

const baseConfig = {
    rules: {
      'no-undef': 'error',
    },
    overrides: [{
        files: ['node.js'],
        env: {
            node: true
        },
    }]
}

const { ESLint } = require("eslint@8.46.0");

const config = { useEslintrc: false, overrideConfig: baseConfig };

await run(ESLint, config, 'classic/node.js'); // should be okay ✅
await run(ESLint, config, 'classic/not-node.js'); // should have an error ✅
// Check output

const { FlatCompat } = require("@eslint/eslintrc@2.1.1");
const { FlatESLint } = require("eslint@8.46.0/use-at-your-own-risk");


const compat = new FlatCompat({
    baseDirectory: __dirname
});
const flatConfig = compat.config(baseConfig);

console.log(flatConfig);

await run(FlatESLint, { overrideConfigFile: true, overrideConfig: flatConfig }, 'flat/node.js'); // should be okay ✅
await run(FlatESLint, { overrideConfigFile: true, overrideConfig: flatConfig }, 'flat/not-node.js'); // should have an error ❌
// Check output

Explanation of the bug

If we have this config:

const legacyConfig = {
    rules: {
      'no-undef': 'error',
    },
    overrides: [{
        files: ['node.js'],
        env: {
            node: true
        },
    }]
}

With the legacy config, using process.cwd() shouldn't be allowed in a file like no-node.js, but allowed in node.js

image

And if we use FlatCompat to port it to the new structure, we should have the same result.

But when we do:

const compat = new FlatCompat({
    baseDirectory: __dirname
});
const flatConfig = compat.config(legacyConfig);

We can see that the files and envs' config aren't merged together, instead we have 1 row for the env, and another one empty one for the overrides:

image

And surely, when we run eslint with this config, no errors are reported (see the "" x2):

image
@Ayc0
Copy link
Author

Ayc0 commented Aug 2, 2023

I don't know if this one is a bug (but it seems related): using compat.env() on multiple envs generates multiple rows, but we may want to only have 1 item with all global merged properly

compat.env({
    node: true,
    es2020: true
})
image

@nzakas nzakas closed this as not planned Won't fix, can't repro, duplicate, stale Aug 3, 2023
@nzakas nzakas added the bug Something isn't working label Aug 3, 2023
@nzakas
Copy link
Member

nzakas commented Aug 3, 2023

I'm sorry, the reproduction case isn't very clear to me. What is it you're trying to show?

For what it's worth, we already know that configs with overrides work, there are a bunch of tests for it:

describe("overrides", () => {

If you'd like to add a test that shows what is failing, we can definitely take a look.

Regarding compat.env(), this is working as expected. It doesn't matter how many objects are created because they are merged at the end. Creating one object per environment makes the logic easier to follow.

@nzakas nzakas reopened this Aug 3, 2023
@Ayc0
Copy link
Author

Ayc0 commented Aug 3, 2023

I'm sorry, the reproduction case isn't very clear to me. What is it you're trying to show?

Using compat.config on a legacy config with env in overrides seem to generate the env outside of the overrides:

const legacyConfig = compat.config({
    rules: {
      'no-undef': 'error',
    },
    overrides: [{
        files: ['node.js'],
        env: {
            node: true
        },
    }]
})

generates something like:

[
  { rules: { no-undef: "error" }, // ✅ correct
  { languageOptions: { globals: {}} }, // ⚠️ applied globally as there are no files
  { files() {  } }, // ⚠️ doesn't contain the globals
]

But my understanding of the flat config is that, as languageOptions is in its own element (without files), it'll be applied globally.
Which seems to the behavior shown in the REPL I linked: with the legacy config, the desired behavior was "on 1 file, eslint passes, and on the other one, it sends an error". But after using compat.config, flat eslint passes on both files.

Instead I'd imagine compat.config to generate this:

[
  { rules: { no-undef: "error" }, // ✅ correct
  { languageOptions: { globals: {}}, files() {  } }, // 👍 has both the files and the languageOptions
]

I can try to modify the test to showcase what I mean.

@Ayc0
Copy link
Author

Ayc0 commented Aug 3, 2023

Regarding compat.env(), this is working as expected. It doesn't matter how many objects are created because they are merged at the end. Creating one object per environment makes the logic easier to follow

That's what I figured. I only mentioned this because this could make working with overrides a bit harder: for instance such code:

const legacyConfig = compat.config({
    overrides: [{
        files: ['node.js'],
        env: {
            node: true,
            es2020: true
        },
    }]
})

This may need to generate:

[
  // The field `files()` is duplicated in both entries
  { languageOptions: { globals: { globals for node }}, files() {  } },
  { languageOptions: { globals: { globals for es2020 }}, files() {  } },
]

@Ayc0
Copy link
Author

Ayc0 commented Aug 3, 2023

I created the test here: https://github.com/eslint/eslintrc/compare/main...Ayc0:eslintrc:Ayc0/potential-bug-investigation?expand=1

If you want, I can open the PR

@nzakas
Copy link
Member

nzakas commented Aug 4, 2023

Thanks, I'll take a look.

As an FYI: the eslintrc-style configs are first translated into an intermediate form (the form that ESLint uses internally) and then into flat config format. The intermediate form doesn't exactly match either eslintrc or flat config formats, which is why there's not always a 1-to-1 matching in either direction.

@nzakas
Copy link
Member

nzakas commented Aug 4, 2023

I've confirmed that there is a bug here: when environments are in an overrides object, the globals are added as a universal config rather than using the same files/excludes as the other options in the config object.

I'm working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted bug Something isn't working
Projects
Archived in project
2 participants