feat: Report config name in error messages #128
Conversation
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.
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.
Ah, that's a good point. Yeah, it makes sense to have the index match what the user sees.
Co-authored-by: Francesco Trotta <[email protected]>
Co-authored-by: Francesco Trotta <[email protected]>
|
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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 <[email protected]>
Co-authored-by: Francesco Trotta <[email protected]>
Co-authored-by: Francesco Trotta <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
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