[PATCH] [10.x] Fix ESLint 6.2.0 + babel-eslint + no-unused-vars false positive with for-in loop

[przemyslawzalewski]

This is a candidate for a PATCH release on the 10.x branch in order to fix the issue with eslint^6.2.0 without introducing any breaking changes.
I have cherry-picked some of the commits from #792 but kept the required changes to the minimum.
I ran tests and lint with eslint^4.12.1, ^5.0.0, ^6.0.0 and ^6.2.0 over this repo and on a project which had the issue with the false-positives.

Fixes:
#791

[przemyslawzalewski]

I had to downgrade eslint dev dependency as the build fails on node 6 (wouldn't it be good to drop support for it with the next major release?) as the ansi-escapes included transitively by eslint@^5.6.0 -> inquirer@^6.2.2 -> ansi-escapes@^4.2.1 declares no support for node 6.

[JLHwung]

I am afraid this PR is too board for a patch release.

Besides, specifying eslint-scope as peerDependencies will introduce unexpected issues and I will explain this later.

What happens in #791

ESLint accepts custom parser plugin. Here is a brief mind map when using ESLint standalone:

                                 -- ast: espree
                                / 
ESLint <--- Parser  --- scopeManager: eslint-scope

And when using eslint + babel-eslint

                                 -- ast: babel-parser + parser-to-espree
                                / 
ESLint <--- Parser  --- scopeManager: eslint-scope

babel-eslint does not introduce a new scopeManager but provide scope manager from eslint-scope. However, babel-eslint still uses eslint-scope 3.7.1 for compatibility of ESLint >= 4.

A breaking change from eslint-scope 3 to 4 is to remove the TDZScope. A year later, ESLint removes TDZScope check on the rule no-unused-vars since ESLint 6 works with eslint-scope 5 which have removed TDZScope long before.

When babel-eslint provides scope manager eslint-scope 3.7 to ESLint 6, A variable declared in TDZScope is apparently a false negative.

The solution, therefore, is straightforward: always use the same eslint-scope version as what eslint specifies in its dependencies:

babel-eslint	eslint	eslint-scope
10	4	3
10	5	4
10	6	5

Why specifying peerDependencies is not a good idea?

Because it will inevitably creates more trouble to our user than this patch tries to solve.

Let's say Lily is using ESLint 4.x and installing babel-eslint 10.0.3-patch, there will be a warning message from package manager, unless she has specified eslint-scope in dependencies.

warning " > babel-eslint@10.0.3-patch" has \
  unmet peer dependency "eslint-scope@^3.7.1 || ^4.0.0 || ^5.0.0".

Now, how could she know which version of eslint-scope she should install? Never to say a warning message on package install is frustrating.

And if she just install eslint-scope as the warning message suggested, she will end up with ESLint 4 + babel-eslint + eslint-scope 5 and we all know ESLint 4 does not work with eslint-scope 5.

Read #793 (comment) for a better solution.

My suggestion

We should remove our eslint-scope in dependencies and do not say anything in peerDependencies.

Note that we will rely on the hoisting behavior of package manager to resolve eslint-scope without specifying dependencies. So we should specify

engines: {
  "npm": ">=3",
}

[feross]

Note that we will rely on the hoisting behavior of package manager to resolve eslint-scope without specifying dependencies. So we should specify

Just want to note that there are alternative package managers like pnpm and others that will break with this assumption.

[JLHwung]

We are considering pin the supported ESLint version of babel-eslint v10 to < 6.2 and roll out babel-eslint v11 which works better with recent ESLint versions.

[przemyslawzalewski]

There is no issue with peer dependencies as you mention. I have published an example repo - https://github.com/przemyslawzalewski/babel-eslint-peer-dependencies - please run npm install to see there is no unmet peer dependency warning as the dependency is provided by the installed eslint package.
I have tested the setup with all supported eslint versions of 10.x (that is eslint^4.12.1, ^5.0.0, ^6.0.0 and ^6.2.0) and it works like a charm.

[nicolo-ribaudo]

Note that we will rely on the hoisting behavior of package manager to resolve eslint-scope without specifying dependencies.

We could do something like this:

var filename = require.resolve(
  "eslint-scope",
  { basedir: require.resolve("eslint") }
);
require(filename);

[otakustay]

We should remove our eslint-scope in dependencies and do not say anything in peerDependencies.

This still introduces issues when eslint has a conflicted eslint-scope resides under its own node_modules folder.

[JLHwung]

I think @nicolo-ribaudo proposed solution is better:

• We do not introduce an extra peerDependency so our user would not be bothered.
• Even one has installed eslint-scope as dependency and thus eslint's eslint-scope resides under its own node_modules, we can still resolve eslint-scope same as what ESLint will resolve
• We don't need to pin eslint-scope for compatibility

nit: basedir is not a valid option for Node.js builtin require.resolve, I guess it comes from browserify/resolve.

@przemyslawzalewski Good to know npm works in this scenario! Could you try if yarn add --dev @przemyslawzalewski/babel-eslint would print peerDependencies warning?

[przemyslawzalewski]

@JLHwung Sadly but yarn raises unmet peer dependency warning:

warning " > @przemyslawzalewski/babel-eslint@10.0.3" has unmet peer dependency "eslint-scope@^3.7.1 || ^4.0.0 || ^5.0.0".

However, everything works without a hassle:

$ yarn run lint
yarn run v1.17.3
$ eslint .
✨  Done in 1.05s.

[hzoo]

Thanks everyone for the help and @przemyslawzalewski for the PRs! Went ahead and ended up going with @JLHwung's PR #794 which uses ESLint's deps instead of going with peerDeps since it really depends on the version being used and we don't want users to have to install it directly on their own.

Comment here: #791 (comment)