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

Projects
None yet
4 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 (#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

Fix: avoid crashing when using baseConfig with extends (fixes #8791)
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.
@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 @gyandeeps, @nzakas and @mysticatea 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 @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();

This comment has been minimized.

@ilyavolodin

ilyavolodin Jun 25, 2017

Member

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.

@ilyavolodin

ilyavolodin Jun 25, 2017

Member

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.

This comment has been minimized.

@not-an-aardvark

not-an-aardvark Jun 25, 2017

Member

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

@not-an-aardvark

not-an-aardvark Jun 25, 2017

Member

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

This comment has been minimized.

@ilyavolodin

ilyavolodin Jun 25, 2017

Member

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.

@ilyavolodin

ilyavolodin Jun 25, 2017

Member

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.

This comment has been minimized.

@not-an-aardvark

not-an-aardvark Jun 25, 2017

Member

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.

@not-an-aardvark

not-an-aardvark Jun 25, 2017

Member

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.

This comment has been minimized.

@not-an-aardvark

not-an-aardvark Jun 25, 2017

Member

Also, it's worth noting that the flattening behavior has been present since 4.0.0, so it's not part of this regression.

@not-an-aardvark

not-an-aardvark Jun 25, 2017

Member

Also, it's worth noting that the flattening behavior has been present since 4.0.0, so it's not part of this regression.

This comment has been minimized.

@ilyavolodin

ilyavolodin Jun 25, 2017

Member

Ok, got it. Sorry, I haven't really looked at this code much since the last refactoring.

@ilyavolodin

ilyavolodin Jun 25, 2017

Member

Ok, got it. Sorry, I haven't really looked at this code much since the last refactoring.

@not-an-aardvark not-an-aardvark merged commit 0d041e7 into master Jun 26, 2017

5 checks passed

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

@not-an-aardvark not-an-aardvark deleted the fix-base-config-extends 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.