-
-
Notifications
You must be signed in to change notification settings - Fork 12
feat: Report config name in error messages #128
Conversation
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.
LGTM, just a couple of suggestions about the JSDoc.
src/config-array.js
Outdated
return this[ConfigArraySymbol.schema].merge(result, this[index]); | ||
} catch (validationError) { | ||
const configName = this[index].name; | ||
const reportedName = typeof configName === 'string' && configName ? configName : index; |
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.
When reporting index
, perhaps the number of objects in the base config should be subtracted from it?
In ESLint, there are 4 objects in default config, so the first object in user's config appears as index 4, which might be misleading.
// eslint.config.js
module.exports = [
{
foo: "bar"
}
];
// Error: Config "4": Unexpected key "foo" found.
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.
Ah, that's a good point. Yeah, it makes sense to have the index match what the user sees.
Co-authored-by: Francesco Trotta <github@fasttime.org>
Co-authored-by: Francesco Trotta <github@fasttime.org>
I updated so that errors occurring in the base config are prefixed with "Base Config" and errors in any other config are prefixed with "Config". That way, the index will make sense given the context. |
src/config-array.js
Outdated
* @private | ||
* @readonly | ||
*/ | ||
this[ConfigArraySymbol.baseConfigLength] = this.length; |
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 could be missing something but it seems that this.length
is always 0 at this point in code when ESLint is launched from the CLI. For the test case indicated in this comment I'm still getting the error message Error: Config "4": Unexpected key "foo" found.
.
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.
Yeah, in ESLint we're actually passing configs
to the ConfigArray
constructor and then inserting baseConfig
(defaultConfig
) afterwards.
I'm not sure why it was done this way. Perhaps we could refactor this in ESLint to pass baseConfig
to the ConfigArray
constructor and then push provided configs
.
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.
One thing that can be confusing is that by "base config" we mean the default config. But there's also ESLint constructor option baseConfig
, which isn't used as a replacement for the default config but added between it and user-provided configs.
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 think we may be getting a bit too wrapped up in the specifics of how this works right now. The goal of this PR was really to expose name
in an error message, and the index is just a fallback in case no name
is specified. We can probably argue for a while about what is the correct fallback and how we may want to refactor FlatConfigArray
to make things make more sense.
For now, I think the best choice is just to do what I had originally: use the actual index in the array and not try to distinguish between base and other configs. Realistically, the contents of a ConfigArray
could change between instantiation and normalization, so any index is just a best guess.
How does that sound?
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.
Would it make sense to just rethrow the unwrapped TypeError
when the config has no name
property? We could follow up with a PR in ESLint to inform users that they are encouraged to add names to their configs in order to make debugging easier (with an info message on the terminal when a TypeError
is detected).
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 agree with @fasttime. As it turns out that including the index in the error message wouldn't be very useful, and in fact rather confusing, it might be better not to include it and just use name
if available.
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.
Okay, if the name isn't present, I'll use "(unknown)" instead to keep the error message format consistent. I'll also attach the index to the error object so we can maybe do something useful with that in ESLint.
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.
We could subtract 4 from the index in ESLint where the number of predefined config elements is known. But I understand from this previous comment that the index is not guaranteed to be preserved during normalization?
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.
That's correct. Let's table any discussions around index for later. I don't think it's important right now, and the data will be available in ESLint if we want to use it for something.
src/config-array.js
Outdated
* @private | ||
* @readonly | ||
*/ | ||
this[ConfigArraySymbol.baseConfigLength] = this.length; |
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.
We could subtract 4 from the index in ESLint where the number of predefined config elements is known. But I understand from this previous comment that the index is not guaranteed to be preserved during normalization?
Co-authored-by: Francesco Trotta <github@fasttime.org>
Co-authored-by: Francesco Trotta <github@fasttime.org>
Co-authored-by: Francesco Trotta <github@fasttime.org>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
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.
LGTM, thanks!
Updates the thrown errors to include the name of the config object. If the name isn't available, then the index in the array is reported.
Refs eslint/eslint#18231
cc @mdjermanovic @fasttime