Eslint-plugin-react: `react/jsx-curly-spacing` rule crashes ESLint 4.2.0

[nwoltman]

It looks like something changed (or broke) in how ESLint handles schemas between 4.1.1 and 4.2.0.

When I try to run ESLint with this rule enabled:

"react/jsx-curly-spacing": [2, {"when": "never", "children": true}]

I get the following error:

can't resolve reference #/definitions/basicConfig from id #
can't resolve reference #/definitions/basicConfigOrBoolean from id #
Warning: [object Object]:
        Configuration for rule "react/jsx-curly-spacing" is invalid:
refVal[2] is not a function Use --force to continue.

Quick link to the react/jsx-curly-spacing schema

Update: I'm guessing eslint/eslint#8852 was the culprit.

[jseminck]

Running tests today also causes the same issue. At first I thought it was related to me being on a Windows machine (haven't been on a Windows machine in a while), but I guess not:

  38) jsx-curly-spacing valid <App foo={42} {...bar} baz={ { 4: 2 } }>
{foo} { { bar: baz } }
</App>:
     Error: rule-tester:
        Configuration for rule "jsx-curly-spacing" is invalid:
refVal[2] is not a function
      at validateRuleOptions (node_modules\eslint\lib\config\config-validator.js:112:15)
      at Object.keys.forEach.id (node_modules\eslint\lib\config\config-validator.js:152:9)
      at Array.forEach (native)
      at validateRules (node_modules\eslint\lib\config\config-validator.js:151:30)
      at Object.validate (node_modules\eslint\lib\config\config-validator.js:206:5)
      at runRuleForItem (node_modules\eslint\lib\testers\rule-tester.js:331:23)
      at testValidTemplate (node_modules\eslint\lib\testers\rule-tester.js:413:28)
      at Context.RuleTester.it (node_modules\eslint\lib\testers\rule-tester.js:546:25)

Looks like some issue with the $ref syntax. I am not really proficient in this... from what I read, everything looks correct.

An easy fix would be to not use $refs -- if that's the way we want to go then let me know and I can raise a PR for it. But on the other hand, it seems like $refs should be supported in the new json validator.

const basicConfig = {
  type: 'object',
  properties: {
    when: {
      enum: SPACING_VALUES
    },
    allowMultiline: {
      type: 'boolean'
    },
    spacing: {
      type: 'object',
      properties: {
        objectLiterals: {
          enum: SPACING_VALUES
        }
      }
    }
  }
};

const basicConfigOrBoolean = {
  oneOf: [
    basicConfig, {
      type: 'boolean'
    }]
};

module.exports = {
  meta: {
    docs: {
      description: 'Enforce or disallow spaces inside of curly braces in JSX attributes',
      category: 'Stylistic Issues',
      recommended: false
    },
    fixable: 'code',

    schema: [{
      oneOf: [{
        allOf: [
          basicConfig, {
            type: 'object',
            properties: {
              attributes: basicConfigOrBoolean,
              children: basicConfigOrBoolean
            }
          }]
      }, {
        enum: SPACING_VALUES
      }]
    }, {
      type: 'object',
      properties: {
        allowMultiline: {
          type: 'boolean'
        },
        spacing: {
          type: 'object',
          properties: {
            objectLiterals: {
              enum: SPACING_VALUES
            }
          }
        }
      },
      additionalProperties: false
    }]
  },

[ljharb]

Refs are great; if they stopped working within the v4 line it's a bug with eslint.

[ljharb]

Please file a bug with them about the breaking change, and link it here?

[nwoltman]

Looks like someone already did - eslint/eslint#8908

[jseminck]

Sorry, I forgot to link it. I asked first on Gitter (totally forgot about that) but then was also suggested to raise an issue on the eslint repo.

Additionally I tried various different approaches to get the schema to pass but no luck (e.g. moving definitions to the root of the object).

[jseminck]

It seems like we should not use $ref inside schema when it is an array due to ESLint internally transforming that array into an object.

It should "work" again when a fix for ajv is released (work meaning: no crash) but the warnings are just being ignored instead of throwing an error.

See these discussions for more info:
https://github.com/eslint/eslint/pull/8852#discussion_r125179364
https://github.com/eslint/eslint/issues/8908#issuecomment-313935248

[ljharb]

I'm not sure I understand what's wrong with the current schema. { $ref: '#/definitions/basicConfig' } is just a macro for { … the basicConfig object }, is it not?

[jseminck]

@ljharb see this comment: https://github.com/eslint/eslint/pull/8852#discussion_r125179222

The schema schema: [obj] is being transformed internally by ESLint to:

{
  type: 'array',
  items: [
    obj
  ],
}

Somehow this messes up the $ref definitions. Why/how exactly I do not fully understand myself. I asked the question on the eslint repo because I would like to understand.

ESLint team fixed a similar issue for the comma-dangle-rule: https://github.com/eslint/eslint/pull/8864

I made a PR with a similar change for eslint-plugin-react.

[ljharb]

so how is that not an eslint bug? it seemed to work fine before.

[jseminck]

Here is the example showing that with is-my-json-valid it was never working correctly: https://runkit.com/esp/596268082d308c00122b7d86. Both options should fail validation ("whatever" is not an allowed value) yet they pass. It seems that when is-my-json-valid cannot resolve $ref it quietly passes validation.

It was working in the sense that the validation error was being ignored. (which means that users could pass an incorrect configuration, and ESLint would accept it as valid)

It should have been the same with the new validator, but there is a bug with it that @epoberezkin will fix: https://github.com/eslint/eslint/issues/8908#issuecomment-313935304

But in the end, it seems better to not have any errors that are being (silently) ignored, right?

[ljharb]

Closed in #1292.

[virgofx]

Can a new build get released with this PR included?

[planttheidea]

Seriously, this was closed nearly a month ago. Is a patch release too much to ask?

[Benno007]

Yes, currently waiting on this if possible

[lukeapage]

@yannickcr sorry to mention you but just in case you aren't aware of people waiting for a release, if you can manage one soon, it would be appreciated, thanks

[yannickcr]

@virgofx @planttheidea @Benno007 @lukeapage Released in v7.2.0, really sorry for the delay.