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.

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

Merged
merged 1 commit into from Jun 26, 2017

Conversation

not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Jun 25, 2017

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 the Config constructor to use the correct context, and to make sure the config cache exists when the baseConfig is evaluated.

Is there anything you'd like reviewers to focus on?

Nothing in particular

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.
@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features patch candidate This issue may necessitate a patch release in the next few days regression Something broke labels Jun 25, 2017
@eslintbot
Copy link

LGTM

@mention-bot
Copy link

@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();
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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features patch candidate This issue may necessitate a patch release in the next few days regression Something broke
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants