Skip to content
This repository has been archived by the owner on Aug 18, 2021. It is now read-only.

fix: require eslint dependencies from eslint base (v10 backport) #794

Merged
merged 1 commit into from
Aug 25, 2019
Merged

fix: require eslint dependencies from eslint base (v10 backport) #794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 8 additions & 4 deletions lib/analyze-scope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
"use strict";

const t = require("@babel/types");
const escope = require("eslint-scope");
const Definition = require("eslint-scope/lib/definition").Definition;
const OriginalPatternVisitor = require("eslint-scope/lib/pattern-visitor");
const OriginalReferencer = require("eslint-scope/lib/referencer");
const requireFromESLint = require("./require-from-eslint");

const escope = requireFromESLint("eslint-scope");
const Definition = requireFromESLint("eslint-scope/lib/definition").Definition;
const OriginalPatternVisitor = requireFromESLint(
"eslint-scope/lib/pattern-visitor"
);
const OriginalReferencer = requireFromESLint("eslint-scope/lib/referencer");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I don't think that to depend on the internal files of ESLint is a good idea because it's not a public API. ESLint can change the internal structure even in patch releases (ESLint can major update espree/eslint-scope in patch releases if it doesn't affect public behaviors). Instead, it's better if babel-eslint depends on own espree/eslint-scope. Those packages follow semantic versioning.

In my 2 cents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESLint can major update espree/eslint-scope in patch releases if it doesn't affect public behaviors

#791 is a scenario when ESLint bumps eslint-scope depedencies and adapts its rule implementation to the new eslint-scope. However, babel-eslint still offers its own old version of eslint-scope, thus the rule returns false positive results since the old eslint-scope does not work with the new rule implementation. (More background on #793 (comment), I am sure you understand the context way better than me)

What would you suggest if babel-eslint would like to support multiple eslint versions? We are still discussing on the version policy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does babel-eslint need to support multiple versions? ESLint does 1 major release a year - could it make sense for babel-eslint to track those releases and also do a major release supporting the new version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @kaicataldo.

We can also restore the ESLint <-> babel-eslint version/support table we had prior to v11 beta.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As just a fact, to depend on internal files of ESLint is really fragile. It's possible to be broken on every ESLint release, including patch releases.

As my opinion, it's not good that babel-eslint adopts such a fragile implementation. I agree with @kaicataldo. I recommend that babel-eslint follows the semantic versioning and update the supported ESLint versions in major versions if needed. The latest eslint-scope should be compatible with eslint@>=5.0.0.

const fallback = require("eslint-visitor-keys").getKeys;
const childVisitorKeys = require("./visitor-keys");

Expand Down
9 changes: 9 additions & 0 deletions lib/require-from-eslint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
"use strict";

const resolve = require("resolve");
const eslintBase = require.resolve("eslint");

module.exports = function requireFromESLint(id) {
const path = resolve.sync(id, { basedir: eslintBase });
return require(path);
};
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
"@babel/parser": "^7.0.0",
"@babel/traverse": "^7.0.0",
"@babel/types": "^7.0.0",
"eslint-scope": "3.7.1",
"eslint-visitor-keys": "^1.0.0"
"eslint-visitor-keys": "^1.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we should not pin eslint-visitor-keys either. But eslint does not import eslint-visitory-keys until 4.14.0. As we have specified eslint peerDependencies to >=4.12.1, I would rather leave it here and then we can remove it on v11.

"resolve": "^1.12.0"
},
"scripts": {
"test": "npm run lint && npm run test-only",
Expand Down
12 changes: 12 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1754,6 +1754,11 @@ path-parse@^1.0.5:
version "1.0.5"
resolved "https://registry.yarnpkg.com/path-parse/-/path-parse-1.0.5.tgz#3c1adf871ea9cd6c9431b6ea2bd74a0ff055c4c1"

path-parse@^1.0.6:
version "1.0.6"
resolved "https://registry.yarnpkg.com/path-parse/-/path-parse-1.0.6.tgz#d62dbb5679405d72c4737ec58600e9ddcf06d24c"
integrity sha512-GSmOT2EbHrINBf9SR7CDELwlJ8AENk3Qn7OikK4nFYAu3Ote2+JYNVvkpAEQm3/TLNEJFD/xZJjzyxg3KBWOzw==

path-type@^2.0.0:
version "2.0.0"
resolved "https://registry.yarnpkg.com/path-type/-/path-type-2.0.0.tgz#f012ccb8415b7096fc2daa1054c3d72389594c73"
Expand Down Expand Up @@ -1894,6 +1899,13 @@ resolve-url@^0.2.1:
version "0.2.1"
resolved "https://registry.yarnpkg.com/resolve-url/-/resolve-url-0.2.1.tgz#2c637fe77c893afd2a663fe21aa9080068e2052a"

resolve@^1.12.0:
version "1.12.0"
resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.12.0.tgz#3fc644a35c84a48554609ff26ec52b66fa577df6"
integrity sha512-B/dOmuoAik5bKcD6s6nXDCjzUKnaDvdkRyAk6rsmsKLipWj4797iothd7jmmUhWTfinVMU+wc56rYKsit2Qy4w==
dependencies:
path-parse "^1.0.6"

resolve@^1.5.0:
version "1.5.0"
resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.5.0.tgz#1f09acce796c9a762579f31b2c1cc4c3cddf9f36"
Expand Down