Skip to content

fix: fix behavior of config-helper.ts when base config contains only meta keys #10755

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

Closed
wants to merge 1 commit into from
Closed

fix: fix behavior of config-helper.ts when base config contains only meta keys #10755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/typescript-eslint/src/config-helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,16 @@ export function config(
...(name && { name }),
};
}),
config,
// The base config should not be included in the final output if it
// consists of only metadata keys (i.e. properties that do not affect the
// final ESLint configuration object)
...(Object.keys(config).every(key =>
Copy link
Member

Choose a reason for hiding this comment

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

This is rather confusing, so maybe deserves a comment - maybe something like this?

// The resulting config objects should include the effect of the base config object 
// with the `extends` key removed so we forward it along as the last object, 
// unless it would be a noop or global ignore

Feel free to word this better than I have 🙂

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 do agree it is confusing, but I can't really think of a more elegant way to express it.

It's possible it may be more readable if the logic wasn't so condensed into that one array such as:

const configArray = [...];

if (
  Object.keys(config).every(key => ['name', 'files', 'ignores'].includes(key))
) {
  return configArray;
} else {
  return configArray.concat(config);
}

But that may also be too verbose, so not sure.

Does the comment sound alright?

Copy link
Member

@kirkwaiblinger kirkwaiblinger Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

... if the logic wasn't so condensed into that one array

Feel free to change this! 🙂 TBH I actually prefer mutating the array for a case like this.

const configArray = [...];

if (
  !Object.keys(config).every(key => ['name', 'files', 'ignores'].includes(key))
) {
  configArray.push(config);
} 

return configArray;

Does the comment sound alright?

My nitpick would be that, as written,

    // The base config should not be included in the final output if it
    // consists of only metadata keys (i.e. properties that do not affect the
    // final ESLint configuration object)

makes it sound like it would be a harmless noop if such a config were included in the output, and we're just doing this cleanliness, but the real reason we're here is that it's actually a harmful operation in the case of global ignores. So if there's a way to convey that 🤷

Copy link
Member

@kirkwaiblinger kirkwaiblinger Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Coming from #10755 (comment), probably best we do this as

function isPossiblyGlobalIgnores(config) {
   Object.keys(config).every(key => ['name', 'ignores'].includes(key)
}

const configArray = [...];

if (
  !isPossiblyGlobalIgnores(config)
) {
  configArray.push(config);
} 

return configArray;

['name', 'files', 'ignores'].includes(key),
)
? []
: // There is some configuration in the base config, so forward it to
// final config array without the `extends` key
[config]),
];
});
}
44 changes: 44 additions & 0 deletions packages/typescript-eslint/tests/config-helper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,4 +247,48 @@ describe('config helper', () => {
{ rules: { rule: 'error' } },
]);
});

it('does not create global ignores in extends', () => {
const configWithIgnores = plugin.config({
extends: [{ rules: { rule1: 'error' } }, { rules: { rule2: 'error' } }],
ignores: ['ignored'],
});

expect(configWithIgnores).toEqual([
{ ignores: ['ignored'], rules: { rule1: 'error' } },
{ ignores: ['ignored'], rules: { rule2: 'error' } },
]);
expect(configWithIgnores).not.toContainEqual(
// Should not create global ignores
{ ignores: ['ignored'] },
);
});

it('does not create noop config in extends', () => {
const configWithMetadata = plugin.config({
extends: [{ rules: { rule1: 'error' } }, { rules: { rule2: 'error' } }],
files: ['file'],
ignores: ['ignored'],
name: 'my-config',
});

expect(configWithMetadata).toEqual([
{
files: ['file'],
ignores: ['ignored'],
name: 'my-config',
rules: { rule1: 'error' },
},
{
files: ['file'],
ignores: ['ignored'],
name: 'my-config',
rules: { rule2: 'error' },
},
]);
expect(configWithMetadata).not.toContainEqual(
// Should not create configuration object with no non-metadata keys (noop config)
{ files: ['file'], ignores: ['ignored'], name: 'my-config' },
);
});
});
Loading