-
-
Notifications
You must be signed in to change notification settings - Fork 239
fix: require eslint dependencies from eslint base (v10 backport) #794
Conversation
@@ -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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so this just imports whatever from eslint vs. our own
Awesome, thanks for the work @JLHwung! Like he said, for everyone's reports/discussion earlier! |
const OriginalPatternVisitor = requireFromESLint( | ||
"eslint-scope/lib/pattern-visitor" | ||
); | ||
const OriginalReferencer = requireFromESLint("eslint-scope/lib/referencer"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
copied fix from PR here: babel#794
Fixes #791
This PR is inspired from the practice and discussion of #793. Thank you @nicolo-ribaudo for the idea and thank you everyone in the community from which I have learned a lot.