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.

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

Jump to bottom

feat: Provide helpful error message for nullish configs #18357

Merged
merged 3 commits into from Apr 19, 2024
Merged

feat: Provide helpful error message for nullish configs #18357

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9baea2
feat: Provide helpful error message for nullish configs
nzakas Apr 17, 2024
4b2490f
Update lib/config/flat-config-array.js
nzakas Apr 18, 2024
2833384
Update lib/config/flat-config-array.js
nzakas Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
101 changes: 101 additions & 0 deletions lib/config/flat-config-array.js
View file
Open in desktop
Expand Up @@ -79,7 +79,53 @@ function getObjectId(object) {
return name;
}

/**
* Wraps a config error with details about where the error occurred.
* @param {Error} error The original error.
* @param {number} originalLength The original length of the config array.
* @param {number} baseLength The length of the base config.
* @returns {TypeError} The new error with details.
*/
function wrapConfigErrorWithDetails(error, originalLength, baseLength) {

let location = "user-defined";
let configIndex = error.index;

/*
* A config array is set up in this order:
* 1. Base config
* 2. Original configs
* 3. User-defined configs
* 4. CLI-defined configs
Comment on lines +95 to +99
Copy link
Member

Choose a reason for hiding this comment

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

A CLI-defined config is what gets passed to the overrideConfig option of the ESLint constructor? When that fails normalization, it's unclear what the index in the error message is meant to be:

import { ESLint } from "eslint";
const eslint = new ESLint({ overrideConfig: [undefined] });
await eslint.lintFiles("*.js");

When running the above code with node in the eslint repo directory, the error message will be

TypeError: Config (unnamed): Unexpected undefined config at user-defined index 24.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also ignorePatterns.

There is no easy way to determine when CLI flags have been added from inside the config array, so I think this error is okay for now.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe configIndex and location (base, original, user-defined, ignore-pattern, etc.) should be added as metadata to each config before normalization, because they are known at that time regardless of the order in which the configs are inserted into the config array. Then, when an error occurs, there would be no need to calculate them back based on the final index.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea! 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

That would require a lot of extra work for very little extra value. Because we're talking about an array, there are many different ways configs could potentially be added.

I think what I have here is good enough for where we are right now. It provides helpful context without much overhead. If we find that this doesn't serve our purpose in the future, we can revisit.

*
* So we need to adjust the index to account for the base config.
*
* - If the index is less than the base length, it's in the base config
* (as specified by `baseConfig` argument to `FlatConfigArray` constructor).
* - If the index is greater than the base length but less than the original
* length + base length, it's in the original config. The original config
* is passed to the `FlatConfigArray` constructor as the first argument.
* - Otherwise, it's in the user-defined config, which is loaded from the
* config file and merged with any command-line options.
*/
if (error.index < baseLength) {
location = "base";
} else if (error.index < originalLength + baseLength) {
location = "original";
configIndex = error.index - baseLength;
} else {
configIndex = error.index - originalLength - baseLength;
}

return new TypeError(
`${error.message.slice(0, -1)} at ${location} index ${configIndex}.`,
{ cause: error }
);
}

const originalBaseConfig = Symbol("originalBaseConfig");
const originalLength = Symbol("originalLength");
const baseLength = Symbol("baseLength");

//-----------------------------------------------------------------------------
// Exports
Expand All @@ -106,12 +152,24 @@ class FlatConfigArray extends ConfigArray {
schema: flatConfigSchema
});

/**
* The original length of the array before any modifications.
* @type {number}
*/
this[originalLength] = this.length;

if (baseConfig[Symbol.iterator]) {
this.unshift(...baseConfig);
} else {
this.unshift(baseConfig);
}

/**
* The length of the array after applying the base config.
* @type {number}
*/
this[baseLength] = this.length - this[originalLength];

/**
* The base config used to build the config array.
* @type {Array<FlatConfig>}
Expand All @@ -129,6 +187,49 @@ class FlatConfigArray extends ConfigArray {
Object.defineProperty(this, "shouldIgnore", { writable: false });
}

/**
* Normalizes the array by calling the superclass method and catching/rethrowing
* any ConfigError exceptions with additional details.
* @param {any} [context] The context to use to normalize the array.
* @returns {Promise<void>} A promise that resolves when the array is normalized.
nzakas marked this conversation as resolved.
Show resolved Hide resolved
*/
normalize(context) {
return super.normalize(context)
.catch(error => {
if (error.name === "ConfigError") {
throw wrapConfigErrorWithDetails(error, this[originalLength], this[baseLength]);
}

throw error;

});
}

/**
* Normalizes the array by calling the superclass method and catching/rethrowing
* any ConfigError exceptions with additional details.
* @param {any} [context] The context to use to normalize the array.
* @returns {void} A promise that resolves when the array is normalized.
nzakas marked this conversation as resolved.
Show resolved Hide resolved
* @throws {TypeError} If the config is invalid.
*/
normalizeSync(context) {

try {

return super.normalizeSync(context);

} catch (error) {

if (error.name === "ConfigError") {
throw wrapConfigErrorWithDetails(error, this[originalLength], this[baseLength]);
}

throw error;

}

}

/* eslint-disable class-methods-use-this -- Desired as instance method */
/**
* Replaces a config with another config to allow us to put strings
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -70,7 +70,7 @@
"@eslint-community/regexpp": "^4.6.1",
"@eslint/eslintrc": "^3.0.2",
"@eslint/js": "9.0.0",
"@humanwhocodes/config-array": "^0.12.3",
"@humanwhocodes/config-array": "^0.13.0",
"@humanwhocodes/module-importer": "^1.0.1",
"@humanwhocodes/retry": "^0.2.3",
"@nodelib/fs.walk": "^1.2.8",
Expand Down
154 changes: 151 additions & 3 deletions tests/lib/config/flat-config-array.js
View file
Open in desktop
Expand Up @@ -191,9 +191,9 @@ async function assertMergedResult(values, result) {
async function assertInvalidConfig(values, message) {
const configs = createFlatConfigArray(values);

await configs.normalize();

assert.throws(() => {
configs.normalizeSync();
configs.getConfig("foo.js");
}, message);
}
Expand Down Expand Up @@ -719,14 +719,162 @@ describe("FlatConfigArray", () => {
describe("Config array elements", () => {
it("should error on 'eslint:recommended' string config", async () => {

await assertInvalidConfig(["eslint:recommended"], "All arguments must be objects.");
await assertInvalidConfig(["eslint:recommended"], "Config (unnamed): Unexpected non-object config at original index 0.");
});

it("should error on 'eslint:all' string config", async () => {

await assertInvalidConfig(["eslint:all"], "All arguments must be objects.");
await assertInvalidConfig(["eslint:all"], "Config (unnamed): Unexpected non-object config at original index 0.");
});


it("should throw an error when undefined original config is normalized", () => {

const configs = new FlatConfigArray([void 0]);

assert.throws(() => {
configs.normalizeSync();
}, "Config (unnamed): Unexpected undefined config at original index 0.");

});

it("should throw an error when undefined original config is normalized asynchronously", async () => {

const configs = new FlatConfigArray([void 0]);

try {
await configs.normalize();
assert.fail("Error not thrown");
} catch (error) {
assert.strictEqual(error.message, "Config (unnamed): Unexpected undefined config at original index 0.");
}

});

it("should throw an error when null original config is normalized", () => {

const configs = new FlatConfigArray([null]);

assert.throws(() => {
configs.normalizeSync();
}, "Config (unnamed): Unexpected null config at original index 0.");

});

it("should throw an error when null original config is normalized asynchronously", async () => {

const configs = new FlatConfigArray([null]);

try {
await configs.normalize();
assert.fail("Error not thrown");
} catch (error) {
assert.strictEqual(error.message, "Config (unnamed): Unexpected null config at original index 0.");
}

});

it("should throw an error when undefined base config is normalized", () => {

const configs = new FlatConfigArray([], { baseConfig: [void 0] });

assert.throws(() => {
configs.normalizeSync();
}, "Config (unnamed): Unexpected undefined config at base index 0.");

});

it("should throw an error when undefined base config is normalized asynchronously", async () => {

const configs = new FlatConfigArray([], { baseConfig: [void 0] });

try {
await configs.normalize();
assert.fail("Error not thrown");
} catch (error) {
assert.strictEqual(error.message, "Config (unnamed): Unexpected undefined config at base index 0.");
}

});

it("should throw an error when null base config is normalized", () => {

const configs = new FlatConfigArray([], { baseConfig: [null] });

assert.throws(() => {
configs.normalizeSync();
}, "Config (unnamed): Unexpected null config at base index 0.");

});

it("should throw an error when null base config is normalized asynchronously", async () => {

const configs = new FlatConfigArray([], { baseConfig: [null] });

try {
await configs.normalize();
assert.fail("Error not thrown");
} catch (error) {
assert.strictEqual(error.message, "Config (unnamed): Unexpected null config at base index 0.");
}

});

it("should throw an error when undefined user-defined config is normalized", () => {

const configs = new FlatConfigArray([]);

configs.push(void 0);

assert.throws(() => {
configs.normalizeSync();
}, "Config (unnamed): Unexpected undefined config at user-defined index 0.");

});

it("should throw an error when undefined user-defined config is normalized asynchronously", async () => {

const configs = new FlatConfigArray([]);

configs.push(void 0);

try {
await configs.normalize();
assert.fail("Error not thrown");
} catch (error) {
assert.strictEqual(error.message, "Config (unnamed): Unexpected undefined config at user-defined index 0.");
}

});

it("should throw an error when null user-defined config is normalized", () => {

const configs = new FlatConfigArray([]);

configs.push(null);

assert.throws(() => {
configs.normalizeSync();
}, "Config (unnamed): Unexpected null config at user-defined index 0.");

});

it("should throw an error when null user-defined config is normalized asynchronously", async () => {

const configs = new FlatConfigArray([]);

configs.push(null);

try {
await configs.normalize();
assert.fail("Error not thrown");
} catch (error) {
assert.strictEqual(error.message, "Config (unnamed): Unexpected null config at user-defined index 0.");
}

});


});

describe("Config Properties", () => {
Expand Down