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.

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

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

Merged
merged 3 commits into from Jun 26, 2017

Conversation

Projects
None yet
7 participants
@not-an-aardvark
Member

not-an-aardvark commented Jun 25, 2017

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 filePath of a resolved config is unique?

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.
@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot commented Jun 25, 2017

LGTM

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jun 25, 2017

@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.

mention-bot commented Jun 25, 2017

@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.

@smably

Sorry to hear about the bugs in this release. I was worried that might happen. Thanks for cleaning up my mess!

Show outdated Hide outdated lib/config/config-file.js
@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot commented Jun 25, 2017

LGTM

@not-an-aardvark

This comment has been minimized.

Show comment
Hide comment
@not-an-aardvark

not-an-aardvark Jun 25, 2017

Member

Sorry to hear about the bugs in this release. I was worried that might happen. Thanks for cleaning up my mess!

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.

Member

not-an-aardvark commented Jun 25, 2017

Sorry to hear about the bugs in this release. I was worried that might happen. Thanks for cleaning up my mess!

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.

@smably

This comment has been minimized.

Show comment
Hide comment
@smably

smably Jun 25, 2017

Contributor

Yes, very complicated, as I discovered even before these bugs appeared!

Contributor

smably commented Jun 25, 2017

Yes, very complicated, as I discovered even before these bugs appeared!

@platinumazure

Just some small comment updates, otherwise this LGTM, thanks!

Show outdated Hide outdated lib/config/config-cache.js
Show outdated Hide outdated lib/config/config-cache.js
@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot commented Jun 26, 2017

LGTM

@not-an-aardvark

This comment has been minimized.

Show comment
Hide comment
@not-an-aardvark

not-an-aardvark Jun 26, 2017

Member

@platinumazure Updated, good catch.

Member

not-an-aardvark commented Jun 26, 2017

@platinumazure Updated, good catch.

@platinumazure

LGTM, thanks!

@ilyavolodin ilyavolodin merged commit f307aa0 into master Jun 26, 2017

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@ilyavolodin ilyavolodin deleted the fix-plugin-config-caching branch Jun 26, 2017

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.