Schema validation modifies exported config object in .eslintrc.js

[andreineculau]

Tell us about your environment

• ESLint Version: 6.7.0
• Node Version: 12.13.1
• npm Version: 6.12.1

What parser (default, Babel-ESLint, etc.) are you using? babel-eslint

Please show your full configuration:

Configuration

// .eslintrc.basic.js
module.exports = {
  plugins: [],

  extends: [
    'eslint:all'
  ],

  parser: 'espree',

  parserOptions: {
    ecmaVersion: 7,
    sourceType: 'module'
  },

  settings: {},

  env: {
    es6: true
  },

  rules: {
    'camelcase': ['warn', {properties: 'never'}]
  }
}

// .eslintrc.babel.js
let _basic = require('./.eslintrc.basic');

module.exports = {
  parser: 'babel-eslint',

  parserOptions: {
    sourceType: 'module'
  },

  plugins: [
    'babel'
  ],

  rules: {
    camelcase: 'off',
    'babel/camelcase': _basic.rules.camelcase
  }
};

// .eslintrc.js
module.exports: {
  extends: [
    './.eslintrc.basic.js',
    './.eslintrc.babel.js'
  ]
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

N/A

N/A

What did you expect to happen?

I expected no errors.

What actually happened? Please include the actual, raw output from ESLint.

	Configuration for rule "babel/camelcase" is invalid:
	Value {"properties":"never","ignoreDestructuring":false,"ignoreImports":false} should NOT have additional properties.

This happens because of #12528 adding ignoreImports with default value failse, and because the defaults of camelcase are merged into the same object in .eslintrc.basic.js:rules.camelcase, which is also the same reference in .eslintrc.babel.js:rules.@babel/camelcase.

Are you willing to submit a pull request to fix this bug?

Sure.

[kaicataldo]

eslint-plugin-babel is maintained by a different team. Please file an issue with https://github.com/babel/eslint-plugin-babel.

[andreineculau]

This has nothing to do with babel or any other plugin. I'm on the mobile now but i think the problem can be reproduced via

let errorConfig = ['error'];
module.exports = {
rules: {camelcase: errorConfig, 'default-case': errorConfig}
};

Where eslint will complain that default-case doesn't know of camelcase's options with default values.

[andreineculau]

correction: let errorConfig = ['error', {}]

[ljharb]

I would expect the json schema to reject an empty object, and to require at least one key to be present. Is that the case here?

[andreineculau]

Eslint barfs with

	Configuration for rule "default-case" is invalid:
	Value {"ignoreDestructuring":false} should NOT have additional properties.

due to configuring ajv with useDefaults: true in

eslint/lib/shared/ajv.js

Line 21 in 219aecb

useDefaults: true,

, which means that the options for camelcase will mutate into {ignoreDestructuring: false}.

I don't think anyone expects the configuration passed to module.exports in a .eslintrc.js file to be mutated. That is why I went ahead and said you eslint should dereference the configuration e.g. config = JSON.parse(JSON.stringify(config)); // do whatever you want with config from here on

My no-plugins example is coerced and doesn't make sense. I don't know why would you want to share the same options to two rules. BUT, now that I have shown you that the behavior is not related to plugins, but to eslint itself, the pattern of sharing configs "upstream" to the babel or typescript plugin is useful because many of their rules are supersets of the built-in ones and it allows you maintain the same configuration.

Real life scenario: babel has a `babel/camelcase` rule, and typescript has a `typescript/camelcase` one, both supersets of the built-in `camelcase` rule. And the recommendation goes like:

{
  camelcase: 'off', // turn off the built-in rule
  'babel/camelcase': ['error', {ignoreDestructuring: true}] // activate the superset rule
}

but if you maintain a .eslintrc.basic.js and a .eslintrc.babel.js meant just for your babel projects, then you need to

• either sync the configuration (i.e. if you want warn instead of error, then make sure to remember to change it in both files)
• or change .eslintrc.babel.js to smth like let basic = require('./eslintrc.basic'); modules.exports = {rules: {camelcase: 'off', 'babel/camelcase': basic.rules.camelcase}};.

[ljharb]

ah, gotcha. your use of the term "dereference" confused me since JS has no dereferencing and no pointers; objects are "reference values" :-)

So specifically, you're saying that the validator should apply to the original config, not to the ajv-mutated one?

[andreineculau]

yup

[ljharb]

This seems important since if unfixed, it leaves inaccurate error messages.

[kaicataldo]

Agreed. Reopened.

[platinumazure]

I've edited the title to describe what I understand the issue to be. Please let me know if I've misunderstood.

I would like to apologize for my previous comment, which might have come off as curt and rude. I was typing while multitasking and I should have waited until I could give my full attention and communicate in the way our valued community members deserve. I failed to do that, and for that I apologize. I have removed the original comment.

[nzakas]

Just for some context, the Ajv docs make it clear that useDefaults modifies original data: https://github.com/ajv-validator/ajv#assigning-defaults

@epoberezkin we seem to be having an issue with the behavior of useDefaults, as we missed that it modifies the object that is passed. Would it be at all feasible for Ajv to have an option to clone the object instead of modifying it? It seems like a more common case to want the original object to remain intact.

[epoberezkin]

@nzakas my decision not to clone in Ajv was because in general case you cannot clone a JavaScript object. You can clone plain objects (e.g. the result of JSON.parse) and quite a few other things, but ajv can be used to validate all types of objects... Doing it in eslint would be easier because eslint can rely on some assumptions about what the object is - whether JSON.parse(JSON.stringify(obj)) is good enough or some deep clone is needed that would correctly clone some other things and not only plain objects.

It seems that these two places need to be changed:

eslint/lib/shared/config-validator.js

Line 96 in 33efd71

ruleValidators.set(rule, ajv.compile(schema));

eslint/lib/shared/config-validator.js

Line 258 in 33efd71

validateSchema = validateSchema || ajv.compile(configSchema);

where ajv.compile(schema) would be replaced with say compile(schema) where compile is something like:

function compile(schema) {
  const validate = ajv.compile(schema);
  return function v(data) {
    const d = clone(data);
    const valid = validate(d);
    v.errors = validate.errors;
    return valid;
  }
}

That is needed as you seem to use ajv infamous api for errors directly... Or you could just return {valid, errors} and refactor the places that consume this result.

[epoberezkin]

@nzakas @kaicataldo let me know what you think - happy to help if needed.

[nzakas]

Yeah, that’s what we are trying to do in #13034

Turns out to be tricky even with assumptions.

[kaicataldo]

Reopening this so we can find another solution.

[btmills]

This was auto-closed when the revert commit was merged. Re-opening since it’s still outstanding.

[nzakas]

I'll tackle this as part of the new config system.

[Nantris]

Holy cow I didn't expect tweaking our default-case commentPattern option to send me down such rabbit hole! Not complaining, just surprised.

Any news on this? Anyone know a workaround to get this config working?

    'default-case': ['error', { commentPattern: /^no-default-case$/i }]

[mdjermanovic]

Any news on this? Anyone know a workaround to get this config working?

    'default-case': ['error', { commentPattern: /^no-default-case$/i }]

@slapbox this isn't related to the schema validation issue, default-case rule by design expects a string value as commentPattern:

'default-case': ['error', { commentPattern: "^no-default-case$" }]

It doesn't support specifying flags. If you want to allow uppercase in all letters, you can use this workaround:

'default-case': ['error', { commentPattern: "^[nN][oO]-[dD][eE][fF][aA][uU][lL][tT]-[cC][aA][sS][eE]$" }]

online demo

[Nantris]

Derp. This was the closest issue I could find at the end of the day with a burned out brain. Big thanks @mdjermanovic!