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

Fix: avoid crashing when using baseConfig with extends (fixes #8791) #8797

Merged
merged 1 commit into from Jun 26, 2017
Merged

Fix: avoid crashing when using baseConfig with extends (fixes #8791) #8797

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/config.js
View file
Open in desktop
Expand Up @@ -66,13 +66,14 @@ class Config {
this.parser = options.parser;
this.parserOptions = options.parserOptions || {};

this.configCache = new ConfigCache();
Copy link
Member

Choose a reason for hiding this comment

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

That's exactly what I've tried earlier today. If you attach a debugger and look at this.baseConfig after it's been resolved, you'll see that extends haven't been resolved and now there are 3 of them there, instead of 1 that's supposed to be. I don't think that's right.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand the issue -- the test asserts that specific values for rules and env are present, which are the expected values if the extends configs have been resolved correctly.

The extends are still present on the baseConfig due to #8636 (it's apparently intended that we don't remove extends from a merged config even after applying the extensions).

Copy link
Member

Choose a reason for hiding this comment

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

Are we also supposed to flatten extends as well? Cause I'm getting 3 extends in baseConfig after resolution, one that was initially there, and 2 from the array config.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. Personally I think it would make more sense to omit extends entirely after the config has already been merged, but that seems like a separate issue.

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, it's worth noting that the flattening behavior has been present since 4.0.0, so it's not part of this regression.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, got it. Sorry, I haven't really looked at this code much since the last refactoring.


this.baseConfig = options.baseConfig
? ConfigOps.merge({}, ConfigFile.loadObject(options.baseConfig, this))
: { rules: {} };
this.baseConfig.filePath = "";
this.baseConfig.baseDirectory = this.options.cwd;

this.configCache = new ConfigCache();
this.configCache.setConfig(this.baseConfig.filePath, this.baseConfig);
this.configCache.setMergedVectorConfig(this.baseConfig.filePath, this.baseConfig);

Expand Down
5 changes: 3 additions & 2 deletions lib/config/config-file.js
View file
Open in desktop
Expand Up @@ -560,11 +560,12 @@ function loadFromDisk(resolvedPath, configContext) {
/**
* Loads a config object, applying extends if present.
* @param {Object} configObject a config object to load
* @param {Config} configContext Context for the config instance
* @returns {Object} the config object with extends applied if present, or the passed config if not
* @private
*/
function loadObject(configObject) {
return configObject.extends ? applyExtends(configObject, "") : configObject;
function loadObject(configObject, configContext) {
return configObject.extends ? applyExtends(configObject, configContext, "") : configObject;
}

/**
Expand Down
18 changes: 18 additions & 0 deletions tests/lib/config.js
View file
Open in desktop
Expand Up @@ -158,6 +158,24 @@ describe("Config", () => {
assert.deepEqual(customBaseConfig, { foo: "bar" });
assert.equal(configHelper.options.format, "foo");
});

it("should create config object when using baseConfig with extends", () => {
const customBaseConfig = {
extends: path.resolve(__dirname, "..", "fixtures", "config-extends", "array", ".eslintrc")
};
const configHelper = new Config({ baseConfig: customBaseConfig }, linter);

assert.deepEqual(configHelper.baseConfig.env, {
browser: false,
es6: true,
node: true
});
assert.deepEqual(configHelper.baseConfig.rules, {
"no-empty": 1,
"comma-dangle": 2,
"no-console": 2
});
});
});

describe("findLocalConfigFiles()", () => {
Expand Down