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

Schema validation modifies exported config object in .eslintrc.js #12592

Closed
andreineculau opened this issue Nov 23, 2019 · 26 comments · Fixed by #13034 or #17666
Closed

Schema validation modifies exported config object in .eslintrc.js #12592

andreineculau opened this issue Nov 23, 2019 · 26 comments · Fixed by #13034 or #17666
Assignees
Labels
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

Comments

@andreineculau
Copy link

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.

@andreineculau andreineculau added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Nov 23, 2019
andreineculau added a commit to tobiipro/eslint-config-firecloud that referenced this issue Nov 23, 2019
@kaicataldo
Copy link
Member

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

@kaicataldo kaicataldo added 3rd party plugin This is an issue related to a 3rd party plugin, config, or parser and removed bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Nov 23, 2019
@andreineculau
Copy link
Author

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
Copy link
Author

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

@ljharb
Copy link
Sponsor Contributor

ljharb commented Nov 23, 2019

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
Copy link
Author

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

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
Copy link
Sponsor Contributor

ljharb commented Nov 23, 2019

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
Copy link
Author

yup

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Dec 24, 2019
@ljharb
Copy link
Sponsor Contributor

ljharb commented Dec 25, 2019

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

@kaicataldo kaicataldo added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed 3rd party plugin This is an issue related to a 3rd party plugin, config, or parser auto closed The bot closed this issue labels Dec 25, 2019
@kaicataldo kaicataldo reopened this Dec 25, 2019
@kaicataldo
Copy link
Member

Agreed. Reopened.

@platinumazure platinumazure changed the title dereference module.exports in .eslintrc.js Schema validation modifies exported config object in .eslintrc.js Dec 29, 2019
@platinumazure
Copy link
Member

platinumazure commented Dec 29, 2019

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.

andreineculau added a commit to tobiipro/eslint-config-firecloud that referenced this issue Jan 12, 2020
# Conflicts:
#	configs/typescript-eslint-recommended.js
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Jan 29, 2020
@kaicataldo kaicataldo self-assigned this Jan 29, 2020
@kaicataldo kaicataldo removed the auto closed The bot closed this issue label Jan 29, 2020
anikethsaha added a commit to anikethsaha/eslint that referenced this issue Mar 25, 2020
anikethsaha added a commit to anikethsaha/eslint that referenced this issue Mar 25, 2020
anikethsaha added a commit to anikethsaha/eslint that referenced this issue Apr 24, 2020
@nzakas
Copy link
Member

nzakas commented May 26, 2020

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
Copy link

epoberezkin commented May 26, 2020

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

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

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
Copy link

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

@nzakas
Copy link
Member

nzakas commented May 27, 2020

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

Turns out to be tricky even with assumptions.

nzakas pushed a commit that referenced this issue Jun 16, 2020
* Fix: clone config data before validation (fixes #12592)

* Chore: added test  and cloning before validation

* Chore: test fix

* Chore: rules tests fixes

* Update: cloning config using json parse and stringify

* Update: changed test description

* Chore: removed unnecessary tests

* Update: changed clone logic

* Update: using lodash.cloneDeepWith for cloning

* Update: handling circular ref cases

* Update: using JSON method for cloning logic

* Update: moved clone logic to normalieObjectConfigData

* Chore: added comments for cloning reason
anikethsaha added a commit to anikethsaha/eslint that referenced this issue Jun 19, 2020
@kaicataldo
Copy link
Member

Reopening this so we can find another solution.

@kaicataldo kaicataldo reopened this Jun 30, 2020
@btmills btmills reopened this Jul 3, 2020
@btmills
Copy link
Member

btmills commented Jul 3, 2020

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

@nzakas nzakas assigned nzakas and unassigned kaicataldo Nov 27, 2020
@nzakas nzakas removed the help wanted The team would welcome a contribution from the community for this issue label Nov 27, 2020
@nzakas
Copy link
Member

nzakas commented Nov 27, 2020

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

@Nantris
Copy link

Nantris commented Sep 14, 2021

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
Copy link
Member

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
Copy link

Nantris commented Sep 15, 2021

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

nzakas added a commit that referenced this issue Oct 19, 2023
In eslint.config.js files, it's possible for two rule configs to contain
references to the same object. That object may be modified during
validation, thus affecting all configs, and may create validation
errors. This clones rule configs before validation to ensure that each
rule is using unique references to manage its configs.

Fixes #12592
mdjermanovic added a commit that referenced this issue Oct 20, 2023
* fix: Ensure shared references in rule configs are separated.

In eslint.config.js files, it's possible for two rule configs to contain
references to the same object. That object may be modified during
validation, thus affecting all configs, and may create validation
errors. This clones rule configs before validation to ensure that each
rule is using unique references to manage its configs.

Fixes #12592

* Update tests/lib/config/flat-config-array.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Update tests/lib/config/flat-config-array.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Update lib/config/flat-config-schema.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Update lib/config/flat-config-schema.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Upgrade config-array and update tests

---------

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
Archived in project
10 participants