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: avoid crashing when using baseConfig with extends (fixes #8791) #8797
Conversation
Due to a bug, an invalid Config instance was getting used when applying extensions to a `baseConfig` object. This updates the `Config` constructor to use the correct context, and to make sure the config cache exists when the `baseConfig` is evaluated.
LGTM |
@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gyandeeps, @nzakas and @mysticatea to be potential reviewers. |
@@ -66,13 +66,14 @@ class Config { | |||
this.parser = options.parser; | |||
this.parserOptions = options.parserOptions || {}; | |||
|
|||
this.configCache = new ConfigCache(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
What is the purpose of this pull request? (put an "X" next to item)
[x] Bug fix (#8791)
What changes did you make? (Give an overview)
Due to a bug, an invalid Config instance was getting used when applying extensions to a
baseConfig
object. This updates theConfig
constructor to use the correct context, and to make sure the config cache exists when thebaseConfig
is evaluated.Is there anything you'd like reviewers to focus on?
Nothing in particular