-
-
Notifications
You must be signed in to change notification settings - Fork 207
fix: require eslint dependencies from eslint base (v10 backport) #794
Conversation
| "@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.
hzoo
left a comment
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.