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

ESLint (2.0.0-alpha-1) should not rewrite the given config object. #4682

Closed
mysticatea opened this Issue Dec 12, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@mysticatea
Copy link
Member

mysticatea commented Dec 12, 2015

Here: https://github.com/eslint/eslint/blob/v2.0.0-alpha-1/lib/eslint.js#L405

config.parserOptions.ecmaFeatures is shallow copied, so L414 overwrites original config.
I encountered incorrect warnings caused by this.

@mysticatea mysticatea self-assigned this Dec 12, 2015

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Dec 12, 2015

huh but config.merge function is supposed to do a deep copy. Are we sure line L405 is the true cause of the issue?

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 12, 2015

I believe.

This deep merge is not deep copy.
When the destination property does not exist, deep merge is assigning the source merely.

https://github.com/eslint/eslint/blob/v2.0.0-alpha-1/lib/config/config-ops.js#L174

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 12, 2015

We can reproduce in this repo's branch: https://github.com/mysticatea/npm-run-all/tree/try-eslint-2a1

  1. clone
  2. checkout try-eslint-2a1
  3. npm i
  4. npm run lint

There is a mix of ES6 and ES5 in this repo, so I use two kind of eslint config.

  • extends: ["mysticatea", "mysticatea/modules", "mysticatea/node"]
  • extends: ["mysticatea/es5", "mysticatea/node"]

mysticatea/node has parserOptions.ecmaFeatures.globalReturn: true but it's rewritten with false in the former, then eslint shows wrong warnings (mainly no-implicit-globals) in the latter.


Hmm, should I fix ConfigOps.merge?

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Dec 12, 2015

if its an issue then we should fix it.

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 12, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 12, 2015

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 13, 2015

Oh, RuleTester freezes config object, so several my plugin's tests are failed here.

image

@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.
You can’t perform that action at this time.