-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
Conversation
Thanks for the PR, @jeremy-code! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
View your CI Pipeline Execution ↗ for commit ddca3e5.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10755 +/- ##
=======================================
Coverage 87.28% 87.29%
=======================================
Files 450 450
Lines 15750 15752 +2
Branches 4613 4613
=======================================
+ Hits 13748 13750 +2
Misses 1645 1645
Partials 357 357
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
549a643
to
3c5bcd2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple questions/changes, but this is a good stuff, thanks for sending!
BTW - you'll want to resolve the lint errors to get CI to pass too, probably yarn lintFix
will be all you need, since I think it's just complaining about the order of fields in objects.
@@ -138,7 +138,11 @@ export function config( | |||
...(name && { name }), | |||
}; | |||
}), | |||
config, | |||
...(Object.keys(config).every(key => |
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 🤷
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we should add logic to handle this type of case too?
it('does not create global ignores when extending empty configs', () => {
expect(
plugin.config({
extends: [{ rules: { rule1: 'error' } }, {}],
ignores: ['ignored'],
}),
).toEqual([
{ ignores: ['ignored'], rules: { rule1: 'error' } },
// Should not create global ignores
// { ignores: ['ignored'] },
// { ignores: ['ignored'] },
]);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is now handled
...
expect(configWithIgnores).not.toContainEqual(
// Should not create global ignores
{ ignores: ['ignored'] },
);
...
expect(configWithMetadata).not.toContainEqual(
// Should not create configuration object with no non-metadata keys (noop config)
{ files: ['file'], ignores: ['ignored'], name: 'my-config' },
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be misunderstanding, but it doesn't appear that this case has been handled? To be clear, the part that I'm trying to highlight is that in the extends
list, there's an empty config object, which will resolve to a { ignores: ['ignored'] }
if we're not careful.
af12070
to
50149a3
Compare
@kirkwaiblinger Thank you for reviewing my PR — I made some changes (added comments, tests). Do you have any thoughts on it as is? |
…y meta keys - `config-helper.ts` only adds base config to config array if it contains keys other than `files`, `ignores`, `names`. - Fixes bug where config helper would create global ignores when extending with base config of `ignores` - Fixes bug where config helper creates extraneous config when extending with base config of only `files`, `ignores`, `names`
50149a3
to
ddca3e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions - also, btw, we like to ask to not force-push since that makes it harder to figure out what has changed since a review, see PR dos and donts 🙏
Main outstanding concern is resolution of #10755 (comment)
@@ -138,7 +138,11 @@ export function config( | |||
...(name && { name }), | |||
}; | |||
}), | |||
config, | |||
...(Object.keys(config).every(key => |
There was a problem hiding this comment.
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 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be misunderstanding, but it doesn't appear that this case has been handled? To be clear, the part that I'm trying to highlight is that in the extends
list, there's an empty config object, which will resolve to a { ignores: ['ignored'] }
if we're not careful.
FYI - eslint core is making their own version of the Note also almost identical logic with excluding the base config object if it's a global ignores. Let's just try and align on the case that the extension is possibly-global-ignores. |
👋 Hey @jeremy-code! Just checking in, is this still something you have time for? No worries if not - I just don't want to leave it hanging. |
Closing this PR as it's been stale for ~6 weeks without activity. Feel free to reopen @jeremy-code if you have time - but no worries if not! If anybody wants to drive it forward, please do post your own PR. If you use this as a start, please add a co-author attribution at the end of your PR description. Thanks! 😊 |
config-helper.ts
only adds base config to config array if it contains keys other thanfiles
,ignores
,names
.ignores
files
,ignores
,names
PR Checklist
config-helper.ts
has unexpected behavior when base config contains only meta keys #10754Overview
Checks if base config only contains the keys
files
,ignores
, ornames
. If so, do not add it to the config array by itself, and only use it for extending. Otherwise, add it to the config array.