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

Gensite fails while building formatters page #8791

Closed
ilyavolodin opened this Issue Jun 24, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@ilyavolodin
Member

ilyavolodin commented Jun 24, 2017

During release today, release script failed while generating formatters page for the site. It looks like glob-based configs introduced a regression for configs that use extend with baseConfig. This branch of code is not covered by unit tests, and it wasn't clear how to fix it during the release. So we skipped generation of formatter page and decided to fix it over the weekend.
We traced the error to this line of code:

return configObject.extends ? applyExtends(configObject, "") : configObject;

Also config is passing two properties here:
? ConfigOps.merge({}, ConfigFile.loadObject(options.baseConfig, this))
but loadObject function only accepts one. Even when I changed the code locally to accept two arguments and pass configContext as second parameter into applyExtends it failed with the same error, because we only add configCache after we call loadObject.

Error message is below:

/var/lib/jenkins/workspace/Releases/ESLint Release/eslint/lib/config/config-file.js:431
            throw e;
            ^

TypeError: Cannot read property 'getConfig' of undefined
Referenced from: undefined
    at load (/var/lib/jenkins/workspace/Releases/ESLint Release/eslint/lib/config/config-file.js:582:51)
    at configExtends.reduceRight.e (/var/lib/jenkins/workspace/Releases/ESLint Release/eslint/lib/config/config-file.js:421:36)
    at Array.reduceRight (native)
    at applyExtends (/var/lib/jenkins/workspace/Releases/ESLint Release/eslint/lib/config/config-file.js:405:28)
    at Object.loadObject (/var/lib/jenkins/workspace/Releases/ESLint Release/eslint/lib/config/config-file.js:567:35)
    at new Config (/var/lib/jenkins/workspace/Releases/ESLint Release/eslint/lib/config.js:70:46)
    at new CLIEngine (/var/lib/jenkins/workspace/Releases/ESLint Release/eslint/lib/cli-engine.js:427:23)
    at getFormatterResults (/var/lib/jenkins/workspace/Releases/ESLint Release/eslint/Makefile.js:467:15)
    at Function.target.gensite (/var/lib/jenkins/workspace/Releases/ESLint Release/eslint/Makefile.js:736:31)
    at Object.global.target.(anonymous function) [as gensite] (/var/lib/jenkins/workspace/Releases/ESLint Release/eslint/node_modules/shelljs/make.js:36:40)
@not-an-aardvark

This comment has been minimized.

Show comment
Hide comment
@not-an-aardvark

not-an-aardvark Jun 24, 2017

Member

Simplified reproduction case:

const eslint = require("eslint");
const cliEngine = new eslint.CLIEngine({ baseConfig: { extends: "eslint:recommended" } });

console.log(cliEngine.executeOnText("foo"));
Expected output
{
  "results": [
    {
      "filePath": "<text>",
      "messages": [
        {
          "ruleId": "strict",
          "severity": 2,
          "message": "Use the global form of 'use strict'.",
          "line": 1,
          "column": 1,
          "nodeType": "Program",
          "source": "foo"
        },
        {
          "ruleId": "no-unused-expressions",
          "severity": 2,
          "message": "Expected an assignment or function call and instead saw an expression.",
          "line": 1,
          "column": 1,
          "nodeType": "ExpressionStatement",
          "source": "foo"
        },
        {
          "ruleId": "no-undef",
          "severity": 2,
          "message": "'foo' is not defined.",
          "line": 1,
          "column": 1,
          "nodeType": "Identifier",
          "source": "foo"
        },
        {
          "ruleId": "eol-last",
          "severity": 2,
          "message": "Newline required at end of file but not found.",
          "line": 1,
          "column": 4,
          "nodeType": "Program",
          "source": "foo",
          "fix": {
            "range": [
              3,
              3
            ],
            "text": "\n"
          }
        },
        {
          "ruleId": "semi",
          "severity": 2,
          "message": "Missing semicolon.",
          "line": 1,
          "column": 4,
          "nodeType": "ExpressionStatement",
          "source": "foo",
          "fix": {
            "range": [
              3,
              3
            ],
            "text": ";"
          }
        }
      ],
      "errorCount": 5,
      "warningCount": 0,
      "fixableErrorCount": 2,
      "fixableWarningCount": 0,
      "source": "foo"
    }
  ],
  "errorCount": 5,
  "warningCount": 0,
  "fixableErrorCount": 2,
  "fixableWarningCount": 0
}

This crashes with the same stack trace as above.

Member

not-an-aardvark commented Jun 24, 2017

Simplified reproduction case:

const eslint = require("eslint");
const cliEngine = new eslint.CLIEngine({ baseConfig: { extends: "eslint:recommended" } });

console.log(cliEngine.executeOnText("foo"));
Expected output
{
  "results": [
    {
      "filePath": "<text>",
      "messages": [
        {
          "ruleId": "strict",
          "severity": 2,
          "message": "Use the global form of 'use strict'.",
          "line": 1,
          "column": 1,
          "nodeType": "Program",
          "source": "foo"
        },
        {
          "ruleId": "no-unused-expressions",
          "severity": 2,
          "message": "Expected an assignment or function call and instead saw an expression.",
          "line": 1,
          "column": 1,
          "nodeType": "ExpressionStatement",
          "source": "foo"
        },
        {
          "ruleId": "no-undef",
          "severity": 2,
          "message": "'foo' is not defined.",
          "line": 1,
          "column": 1,
          "nodeType": "Identifier",
          "source": "foo"
        },
        {
          "ruleId": "eol-last",
          "severity": 2,
          "message": "Newline required at end of file but not found.",
          "line": 1,
          "column": 4,
          "nodeType": "Program",
          "source": "foo",
          "fix": {
            "range": [
              3,
              3
            ],
            "text": "\n"
          }
        },
        {
          "ruleId": "semi",
          "severity": 2,
          "message": "Missing semicolon.",
          "line": 1,
          "column": 4,
          "nodeType": "ExpressionStatement",
          "source": "foo",
          "fix": {
            "range": [
              3,
              3
            ],
            "text": ";"
          }
        }
      ],
      "errorCount": 5,
      "warningCount": 0,
      "fixableErrorCount": 2,
      "fixableWarningCount": 0,
      "source": "foo"
    }
  ],
  "errorCount": 5,
  "warningCount": 0,
  "fixableErrorCount": 2,
  "fixableWarningCount": 0
}

This crashes with the same stack trace as above.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Jun 24, 2017

Member

Unit tests for this error is in config.js:

it("should create config object when using baseConfig with extends", () => {
    const customBaseConfig = { extends: path.resolve(__dirname, "..", "fixtures", "config-extends", "array", ".eslintrc") },
    configHelper = new Config({ baseConfig: customBaseConfig }, linter);

    assert.equal(configHelper.baseConfig.rules["no-empty"], 2);
});

However, I still can't figure out what should be the right behavior. We keep trying to deal with configCache while loading extends, but at that point, it still hasn't been created.

Member

ilyavolodin commented Jun 24, 2017

Unit tests for this error is in config.js:

it("should create config object when using baseConfig with extends", () => {
    const customBaseConfig = { extends: path.resolve(__dirname, "..", "fixtures", "config-extends", "array", ".eslintrc") },
    configHelper = new Config({ baseConfig: customBaseConfig }, linter);

    assert.equal(configHelper.baseConfig.rules["no-empty"], 2);
});

However, I still can't figure out what should be the right behavior. We keep trying to deal with configCache while loading extends, but at that point, it still hasn't been created.

@not-an-aardvark

This comment has been minimized.

Show comment
Hide comment
@not-an-aardvark

not-an-aardvark Jun 24, 2017

Member

Does it make sense to store the baseConfig in the configCache at all? I had thought the cache was only useful for configs read from the filesystem, but I'll have to look at the PR again to double check. (#8792 is also caused by an issue with the cache.)

Member

not-an-aardvark commented Jun 24, 2017

Does it make sense to store the baseConfig in the configCache at all? I had thought the cache was only useful for configs read from the filesystem, but I'll have to look at the PR again to double check. (#8792 is also caused by an issue with the cache.)

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Jun 24, 2017

Member

Right, but it goes through the same code as other configs do as well. I tried to change the logic in the morning by adding checks for empty configContext, but I think something else is wrong, since the extends doesn't get resolved correctly.

Member

ilyavolodin commented Jun 24, 2017

Right, but it goes through the same code as other configs do as well. I tried to change the logic in the morning by adding checks for empty configContext, but I think something else is wrong, since the extends doesn't get resolved correctly.

@not-an-aardvark not-an-aardvark self-assigned this Jun 24, 2017

not-an-aardvark added a commit that referenced this issue Jun 25, 2017

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.

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