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

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Aug 24, 2019

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.

@@ -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.

Copy link
Member

@hzoo hzoo left a 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

@hzoo hzoo merged commit 354953d into babel:10.x Aug 25, 2019
@JLHwung JLHwung deleted the fix-791-v10 branch August 25, 2019 20:58
@hzoo
Copy link
Member

hzoo commented Aug 25, 2019

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");
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.

jharris4 added a commit to jharris4/babel-eslint that referenced this pull request Sep 10, 2019
copied fix from PR here:
babel#794
This was referenced Jun 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants