Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix: ensure configs from a plugin are cached separately (fixes #8792) (
…#8798)

* Fix: ensure configs from a plugin are cached separately (fixes #8792)

Config files are cached by file path to avoid needing to load the same config file twice. However, configs provided by plugins all have the same file path (the index file of the plugin). This created a bug when loading multiple configs for the same plugin where only the highest-precedence config from that plugin would be loaded. All other configs from that plugin would be considered identical by the cache, so they would all end up getting pulled from the cache as the same config.

This commit updates the caching logic to use the config full name as a cache index. This is still the file path when resolving a config from the filesystem, but it is the unique plugin config identifier (e.g. 'plugin:foo/node-config') when resolving a plugin config.

* Remove unused second argument from loadConfigFile call

* Fix inaccurate comments for config-cache methods
  • Loading branch information
not-an-aardvark authored and ilyavolodin committed Jun 26, 2017
1 parent 8b48ae8 commit f307aa0
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 12 deletions.
16 changes: 9 additions & 7 deletions lib/config/config-cache.js
Expand Up @@ -29,31 +29,33 @@ function hash(vector) {
module.exports = class ConfigCache {

constructor() {
this.filePathCache = new Map();
this.configFullNameCache = new Map();
this.localHierarchyCache = new Map();
this.mergedVectorCache = new Map();
this.mergedCache = new Map();
}

/**
* Gets a config object from the cache for the specified config file path.
* @param {string} configFilePath the absolute path to the config file
* @param {string} configFullName the name of the configuration as used in the eslint config(e.g. 'plugin:node/recommended'),
* or the absolute path to a config file. This should uniquely identify a config.
* @returns {Object|null} config object, if found in the cache, otherwise null
* @private
*/
getConfig(configFilePath) {
return this.filePathCache.get(configFilePath);
getConfig(configFullName) {
return this.configFullNameCache.get(configFullName);
}

/**
* Sets a config object in the cache for the specified config file path.
* @param {string} configFilePath the absolute path to the config file
* @param {string} configFullName the name of the configuration as used in the eslint config(e.g. 'plugin:node/recommended'),
* or the absolute path to a config file. This should uniquely identify a config.
* @param {Object} config the config object to add to the cache
* @returns {void}
* @private
*/
setConfig(configFilePath, config) {
this.filePathCache.set(configFilePath, config);
setConfig(configFullName, config) {
this.configFullNameCache.set(configFullName, config);
}

/**
Expand Down
13 changes: 8 additions & 5 deletions lib/config/config-file.js
Expand Up @@ -488,12 +488,15 @@ function normalizePackageName(name, prefix) {
* @returns {Object} An object containing 3 properties:
* - 'filePath' (required) the resolved path that can be used directly to load the configuration.
* - 'configName' the name of the configuration inside the plugin.
* - 'configFullName' the name of the configuration as used in the eslint config (e.g. 'plugin:node/recommended').
* - 'configFullName' (required) the name of the configuration as used in the eslint config(e.g. 'plugin:node/recommended'),
* or the absolute path to a config file. This should uniquely identify a config.
* @private
*/
function resolve(filePath, relativeTo) {
if (isFilePath(filePath)) {
return { filePath: path.resolve(relativeTo || "", filePath) };
const fullPath = path.resolve(relativeTo || "", filePath);

return { filePath: fullPath, configFullName: fullPath };
}
let normalizedPackageName;

Expand All @@ -510,7 +513,7 @@ function resolve(filePath, relativeTo) {
normalizedPackageName = normalizePackageName(filePath, "eslint-config");
debug(`Attempting to resolve ${normalizedPackageName}`);
filePath = resolver.resolve(normalizedPackageName, getLookupPath(relativeTo));
return { filePath };
return { filePath, configFullName: filePath };


}
Expand Down Expand Up @@ -580,7 +583,7 @@ function loadObject(configObject, configContext) {
function load(filePath, configContext, relativeTo) {
const resolvedPath = resolve(filePath, relativeTo);

const cachedConfig = configContext.configCache.getConfig(resolvedPath.filePath);
const cachedConfig = configContext.configCache.getConfig(resolvedPath.configFullName);

if (cachedConfig) {
return cachedConfig;
Expand All @@ -591,7 +594,7 @@ function load(filePath, configContext, relativeTo) {
if (config) {
config.filePath = resolvedPath.filePath;
config.baseDirectory = path.dirname(resolvedPath.filePath);
configContext.configCache.setConfig(resolvedPath.filePath, config);
configContext.configCache.setConfig(resolvedPath.configFullName, config);
}

return config;
Expand Down
3 changes: 3 additions & 0 deletions tests/fixtures/config-file/plugins/.eslintrc2.yml
@@ -0,0 +1,3 @@
extends:
- plugin:test/foo
- plugin:test/bar
43 changes: 43 additions & 0 deletions tests/lib/config/config-file.js
Expand Up @@ -899,6 +899,49 @@ describe("ConfigFile", () => {
}
});
});

it("should load two separate configs from a plugin", () => {
const stubConfig = new Config({}, new Linter());
const resolvedPath = path.resolve(PROJECT_PATH, "./node_modules/eslint-plugin-test/index.js");

const configDeps = {
"../util/module-resolver": createStubModuleResolver({
"eslint-plugin-test": resolvedPath
}),
"require-uncached"(filename) {
return configDeps[filename];
}
};

configDeps[resolvedPath] = {
configs: {
foo: { rules: { semi: 2, quotes: 1 } },
bar: { rules: { quotes: 2, yoda: 2 } }
}
};

const StubbedConfigFile = proxyquire("../../../lib/config/config-file", configDeps);

const configFilePath = getFixturePath("plugins/.eslintrc2.yml");
const config = StubbedConfigFile.load(configFilePath, stubConfig);

assert.deepEqual(config, {
baseDirectory: path.dirname(configFilePath),
filePath: configFilePath,
parserOptions: {},
globals: {},
env: {},
rules: {
semi: 2,
quotes: 2,
yoda: 2
},
extends: [
"plugin:test/foo",
"plugin:test/bar"
]
});
});
});

describe("even if config files have Unicode BOM,", () => {
Expand Down

0 comments on commit f307aa0

Please sign in to comment.