Inconsistent rule configuration with new eslint.CLIEngine

[vladikoff]

I'm trying to make a smaller proof of bug here, but here is what I have so far:

STR

git clone https://github.com/mozilla/fxa-content-server#e29f5d654e8359051507314fb14efcf1e109bf46
cd fxa-content-server
npm install

1) Run

node_modules/.bin/grunt eslint

runs all tasks (including the one below), everything passes

2) Run

node_modules/.bin/grunt eslint:tests

Throws the error:

tests/functional/force_auth.js
  23:2  error  Designated alias 'self' is not assigned to 'this'  consistent-this
  32:2  error  Designated alias 'self' is not assigned to 'this'  consistent-this

✖ 2 problems (2 errors, 0 warnings)

End of STR

From my investigation the options that are passed via the grunt task in https://github.com/sindresorhus/grunt-eslint/blob/master/tasks/eslint.js#L46 are identical

The traversal has a different rule setting in 1) and 2):

Grunt task configuration here: https://github.com/mozilla/fxa-content-server/blob/master/grunttasks/eslint.js#L24

At this moment it seems that eslint has some global state of linting rules that merges itself instead of being independent for each var engine = new eslint.CLIEngine(opts);

[ilyavolodin]

Are you saying this is specific to CLIEngine? Or do you have the same issue if you bypass grunt and run eslint directly?

[vladikoff]

@pdehaan is there a way to reproduce this via the command line? I have a feeling that the non-programmatic way will not have this issue

[vladikoff]

@pdehaan what could be happening here: the eslint:config Merging command line rules +0ms

the merge that is actually happening: main task options -> app task options -> tests task option the merge that should be happening: main task options -> tests task option

[pdehaan]

I'm able to reproduce this with ESLint in the above repo (fxa-content-server) using the following scripts in my package.json:

  "scripts": {
    "lint": "node_modules/grunt-eslint/node_modules/.bin/eslint .",
    "lint-tests": "node_modules/grunt-eslint/node_modules/.bin/eslint tests",
    "lint-version": "node_modules/grunt-eslint/node_modules/.bin/eslint --version",
    // ...
  }

---

Output:

$ npm run lint-version

> fxa-content-server@0.41.1 lint-version /Users/pdehaan/dev/tmp/del/fxa-content-hook-lint
> node_modules/grunt-eslint/node_modules/.bin/eslint --version

v0.24.0

$ npm run lint

> fxa-content-server@0.41.1 lint /Users/pdehaan/dev/tmp/del/fxa-content-hook-lint
> node_modules/grunt-eslint/node_modules/.bin/eslint .

$ npm run lint-tests

> fxa-content-server@0.41.1 lint-tests /Users/pdehaan/dev/tmp/del/fxa-content-hook-lint
> node_modules/grunt-eslint/node_modules/.bin/eslint tests

tests/functional/force_auth.js
  23:2  error  Designated alias 'self' is not assigned to 'this'  consistent-this
  32:2  error  Designated alias 'self' is not assigned to 'this'  consistent-this

tests/functional/oauth_force_email.js
  27:2  error  Designated alias 'self' is not assigned to 'this'  consistent-this

✖ 3 problems (3 errors, 0 warnings)

[ilyavolodin]

@pdehaan Thanks! That helps. At least we can rule out grunt-eslint as a source of the issue. Could you possibly run the same tasks with debugging on? It looks like tests linting is picking up an extra .eslintrc file somewhere.

[pdehaan]

@ilyavolodin Done. It was a bit more, verbose, than I was expecting, but you can view the Gist at https://gist.github.com/pdehaan/128328cef39125d34de2

To make it a bit simpler, I did an npm i eslint@latest -D and just ran ESLint directly via npm run {script}, rather than proxy into ./node_modules/grunt-eslint/node_modules/.bin/eslint.

---

Here's my revised package.json. Let me know if you want me to reduce the debug target, instead of DEBUG=eslint:*:

  "scripts": {
    "lint": "DEBUG=eslint:* eslint .",
    "lint-tests": "DEBUG=eslint:* eslint tests",
    "lint-version": "DEBUG=eslint:* eslint --version",
    // ...
  }

[gyandeeps]

@ilyavolodin I think I have an hint of whats happening here. our util.mergeConfigs function is doing a shallow copy and not deep copy. That why the configs are getting modified here.

To prove do the following steps:

var a = {a:1, b:{x:1, y: 2}};
var b = {r:3, a:5, b:{x:8, y: 2, z:99}}

var config = mergeConfigs(a, b);

console.log(a); // the values would have changed to 

I tried it on chrome console.

[ilyavolodin]

@gyandeeps That would do it. But how come we never noticed this before? I would imagine a lot of people would've run into this issue if that was the case.

[pdehaan]

Sweet, do we win a prize?

Not sure why more people wouldn't have run into it, but we're using the new shareable configs, plus some inherited .eslintrc files in our project, and a few different Grunt eslint targets with different configs, so maybe we're just doing some cutting edge stuff here.

[ilyavolodin]

It's possible... But I actually think we have unit tests to verify that we are not overriding configs... Very strange, but I guess we'll have to take a closer look. Thanks for report.

[gyandeeps]

I am working on it but I would be great to have the changes once tested by @pdehaan on his cutting edge scenario. That would help us a lot. and actually tell us whether thats the core problems or should be start looking else where. @pdehaan would that be fine with you?

[pdehaan]

Yeah, we're here to help. Let me know when you have something pushed to GitHub and we can try syching to that Git SHA and see if it solves the issue (I think we renamed those vars already in our repo to silence the consistent-this errors, but I can revert them back locally to make sure that everything [failis and then] passes).

[gyandeeps]

@pdehaan Can you just pick up just my modifications from here and add them to your version of eslint . And then run the tests again as you ran before. Thanks for help :)

Note: Just pick my changes and not the whole file.

let me know if you want me to give you 0.24.0 version with my changes in it.

[gyandeeps]

@pdehaan Can you also share you .eslintrc files, both

• Located in folder fxa-content-hook-lint
• Located in folder fxa-content-hook-lint -> app

Also if I missing any other (I got the above 2 based on your gist)

Also if you have any configs inside package json, please share those also. Thanks I am trying to recreate this scenario locally.

[pdehaan]

@gyandeeps Yeah, the entire project is at https://github.com/mozilla/fxa-content-server

• https://github.com/mozilla/fxa-content-server/blob/master/.eslintrc
• https://github.com/mozilla/fxa-content-server/blob/master/app/.eslintrc

And both inherit from our shareable config at:

• https://github.com/pdehaan/eslint-config-fxa

[gyandeeps]

Awesome so I can basically checkout the project and check my changes. Thanks

[pdehaan]

@gyandeeps: I grabbed those 2 modified files from your commit and replaced my local versions in ./node_modules/eslint/lib/{eslint,util}.js and re-ran the tests.

Now I get consistent failures in npm run lint and npm run lint-tests. :+1:

[gyandeeps]

Hence proved that the issue we thought is the real issue. Thanks a lot @pdehaan for help and testing. :+1:

[pdehaan]

@gyandeeps @ilyavolodin Thanks for the quick turnaround. Cant wait for eslint@1.0.0!

[nzakas]

Nice investigation, all! I think this probably warrants a 0.24.1 release due to the severity. I'll put that together tomorrow.

[vladikoff]

Thanks so much for triaging and fixing the issue so quickly, really appreciate it :fireworks: