Fix: ensure configs from a plugin are cached separately (fixes #8792) #8798
Conversation
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.
not-an-aardvark
added
accepted
|
LGTM |
|
@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @mysticatea and @smably to be potential reviewers. |
lib/config/config-file.js
Outdated
| @@ -524,7 +527,7 @@ function resolve(filePath, relativeTo) { | |||
| function loadFromDisk(resolvedPath, configContext) { | |||
| const dirname = path.dirname(resolvedPath.filePath), | |||
| lookupPath = getLookupPath(dirname); | |||
| let config = loadConfigFile(resolvedPath); | |||
| let config = loadConfigFile(resolvedPath, configContext); | |||
There was a problem hiding this comment.
loadConfigFile doesn't seem to take a second argument. Was this change deliberate?
There was a problem hiding this comment.
Oops, this was unintentional, thanks. While working on this I initially considered adding a second argument to loadConfigFile, but then I forgot to remove it here.
|
LGTM |
No worries -- the codebase probably should have had more tests for this even before you started working on it. 😅 Configs are complicated. It's hard to tell whether a change will break something if it's untested, especially if you haven't worked on the codebase before. |
|
Yes, very complicated, as I discovered even before these bugs appeared! |
lib/config/config-cache.js
Outdated
| 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 absolute path to the config file |
There was a problem hiding this comment.
Could this line be updated to match lib/config/config-file.js lines 491-492?
lib/config/config-cache.js
Outdated
| } | ||
|
|
||
| /** | ||
| * 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 absolute path to the config file |
There was a problem hiding this comment.
Could this line be updated to match lib/config/config-file.js lines 491-492?
|
LGTM |
|
@platinumazure Updated, good catch. |
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[x] Bug fix (#8792)
What changes did you make? (Give an overview)
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.
Is there anything you'd like reviewers to focus on?
Are there any other places where we assume that the
filePathof a resolved config is unique?