Update: improve parser integrations #542

mysticatea · 2017-11-22T04:34:59Z

This is a verification for eslint/eslint#8755.

This PR prepares to remove the monkey patching by using the experimental feature eslint/eslint#8755. It uses eslint/eslint#8755 if it's landed on the current running ESLint, otherwise, it uses the monkey patching as same as current.

This PR does not refactor the monkey patching implementation. This just moved it to lib/parse-with-patch.js. We can remove it in future. As the result, the monkey patching gets gone.

This PR reimplements the patch of scope analysis in lib/analyze-scope.js with ES2015 classes. This implementation still depends on the internal code of eslint-scope, but we can control the version of the eslint-scope via package.json (~3.7.1 in this PR), so it reduces broken risk. We should continue to tackle for extensible scope analysis module.

Anyway, I confirmed that babel-eslint can be implemented with eslint/eslint#8755 without monkey patching. Please don't merge this PR until the experimental feature is frozen.

cc: @hzoo, @kaicataldo, @JamesHenry

JamesHenry · 2017-11-22T15:37:50Z

Did you forget about #509? With your support it would have been possible to get that one over the line I think

mysticatea · 2017-11-23T04:35:00Z

I'm sorry.
I think backward compatibility is important, so #509 was not the best option to start. Also, I needed to get knowledge about the original implementation because some tests are failing in #509.

JamesHenry · 2017-11-23T17:03:01Z

Static text is a tricky medium, so I want to be super clear straight off the bat that there is no animosity or upset from my side, and I hope I am not coming across as being precious about my code vs yours. That is definitely not the case and I know it was not your intention to dismiss the existing work, you just want to find the best solution.

The topic of backwards compatibility did come up that PR and @hzoo had said that it was not a constraint #509 (comment)

It's totally reasonable for you to have a different point of view on that, and I think we could have discussed your concerns with dropping backwards compability on the thread for that PR (and we may well have agreed that the best approach was to be backwards compatible in the end!), rather than starting from scratch.

I just think in the long term, it's not a scalable approach that if people have different views on the same solution they develop their own alternative PRs, rather than discussing the differences on existing ones.

What do we think is the best way forward from here? The way I see it now, the topic of backwards compatibility needs to be decided on once and for all and a single PR needs to be finalised for review by some other team members. What do you think?

mysticatea · 2017-11-23T19:21:27Z

Writing English is hard (need hours) to me, some things are lacking in my previous comment.
One is that I didn't have the knowledge how babel-eslint is implemented. So it was difficult to start from WIP point. #509 has some failing tests, some conflicts, and depending on WIP branch of other packages. I had to learn with my hand how babel-eslint is implemented from a stable point.
Another one is that I wanted to confirm whether eslint/eslint#8755 is worth truly or not. I was worrying that we cannot delete the monkey patching by the feature because #509 was stopped on having failing tests while a long time. So I thought it's a good theme in order to learn babel-eslint.
This implementation is the result of my learning about babel-eslint, I don't mind if abandoned. I think you can use lib/analyze-scope.js of this PR to complete #509.

JamesHenry · 2017-11-24T22:16:34Z

That makes sense, thanks a lot for explaining the background! 👍

I will take a look at both PRs with fresh eyes and propose a path forward for your and others to review.

As an aside, your English is awesome and I'm really grateful for all of the extra effort you put in to be such an effective contributor and leader in your second language!

# Conflicts: # package.json

mysticatea · 2017-12-07T22:25:19Z

eslint/eslint#8755 has been accepted at the last TSC meeting, so I removed [WIP] mark.

JamesHenry · 2017-12-07T23:17:47Z

Excellent, then let's press ahead with this PR. I will close mine out, as I am finding my time very limited at the moment and you are on top of this.

kaicataldo · 2017-12-07T23:21:46Z

@JamesHenry Appreciate all the work you've done on this!

mysticatea · 2017-12-21T00:15:10Z

eslint/eslint#8755 has been merged.

@hzoo Could you review this PR?

not-an-aardvark · 2017-12-24T01:50:18Z

ESLint v4.14.0 was released today, with the new API for custom scope analysis. However, it also seems to be breaking the existing babel-eslint scope analysis monkeypatches (see eslint/eslint#9762).

When I installed babel-eslint from this PR with the reproduction case in eslint/eslint#9762 (comment), I found that a minor bug in this PR was causing a module resolution error. I pushed b05e94e to this PR to fix the issue. (This wasn't caught by the tests because babel-types is installed from a devDependency, but is not installed from any dependencies.)

With that change applied, I can confirm that this PR fixes the issue in eslint/eslint#9762, so it would be good to include it in a release soon.

Is this PR a breaking change for babel-eslint? If not, then releasing a new version of babel-eslint should be a good solution for the problem in eslint/eslint#9762. If this is a breaking change for babel-eslint, I can look into the logic in eslint to see if we can find a way to keep the old monkeypatching behavior working.

not-an-aardvark · 2017-12-24T02:15:46Z

It looks like it would be difficult to fix eslint/eslint#9762 in ESLint, because babel-eslint currently relies on monkeypatching ESLint's visitor keys from estraverse, but ESLint doesn't depend on estraverse anymore at all. So ESLint would have to add estraverse as a dependency and try to detect monkeypatches to it, which seems undesirable.

However, I've verified that this change is backwards-compatible, because it feature-detects newer versions of ESLint here and falls back to the old implementation for older ESLint versions. So I think it should be fine to merge this as a fix for eslint/eslint#9762.

platinumazure · 2017-12-24T05:07:51Z

package.json

-    "babylon": "7.0.0-beta.31"
+    "babylon": "7.0.0-beta.31",
+    "eslint-scope": "~3.7.1",
+    "eslint-visitor-keys": "^0.1.0"


@mysticatea Should this be edited to ^1.0.0?

@platinumazure I just pushed a commit to update this.

hzoo · 2017-12-24T05:09:24Z

If people are comfortable, can feel free to merge/release (can add everyone, and I'l do it tomorrow if i have time otherwise)

ok added @not-an-aardvark @mysticatea @kaicataldo @JamesHenry @zertosh on npm (should already be collabs for this repo on github too)

mysticatea · 2017-12-24T06:16:22Z

Thank you @not-an-aardvark for the update! LGTM.

kaicataldo

Aside from an unused options property passed to Babylon, LGTM!

kaicataldo · 2017-12-24T06:25:32Z

lib/parse-with-scope.js

+
+module.exports = function(code, options) {
+  options = options || {};
+  options.ecmaVersion = options.ecmaVersion || 6;


I don’t think this is used anywhere else (it looks unused in the parse method that is called with these defined options). Babylon doesn’t really have a concept of ES versions - it always is able to parse everything.

This is to rewrite the options object directly because some core rules depends on parserOptions.ecmaVersion.

Ah I see. In that case, We should probably set this to the latest version ESLint core rules support.

As discussed in the ESLint Gitter, I'll make a followup PR for this.

kaicataldo · 2017-12-24T06:28:14Z

lib/analyze-scope.js

+
+class Referencer extends OriginalReferencer {
+  // inherits.
+  visitPattern(node, options, callback) {


Does super.visitPattern not need to be called in here somewhere?

Never mind, just read the comment.

I copied the body of super.visitPattern to use inherited PatternVisitor.

(fixes eslint/eslint#9762)

mysticatea added 2 commits November 17, 2017 21:20

fix prettier errors

fb5e3c9

Update: improve parser integrations

fbabd73

mysticatea mentioned this pull request Nov 22, 2017

Update: Improve parser integrations (fixes #8392) eslint/eslint#8755

Merged

fix a mistake

5791dd1

mysticatea changed the title ~~[WIP] Update: improve parser integrations~~ Update: improve parser integrations Dec 7, 2017

mysticatea added 3 commits December 8, 2017 07:14

Merge branch 'master' of https://github.com/babel/babel-eslint

e002886

Merge branch 'master' into eslint-improved-parser-integration2

ae6c649

# Conflicts: # package.json

fix eslint dep

bb0f0b7

JamesHenry mentioned this pull request Dec 7, 2017

Breaking: Replace parse with parseForESLint and refactor codebase #509

Closed

mysticatea mentioned this pull request Dec 8, 2017

'loc' of null error when using babel-eslint as a custom parser vuejs/vue-eslint-parser#21

Closed

Use @babel/types rather than babel-types in visitor-keys

b05e94e

not-an-aardvark approved these changes Dec 24, 2017

View reviewed changes

not-an-aardvark mentioned this pull request Dec 24, 2017

v4.14.0 scope analysis issue with babel-eslint eslint/eslint#9762

Closed

platinumazure reviewed Dec 24, 2017

View reviewed changes

Upgrade eslint-visitor-keys to ^1.0.0

b433d28

not-an-aardvark force-pushed the eslint-improved-parser-integration2 branch from 60e29be to b433d28 Compare December 24, 2017 05:38

kaicataldo approved these changes Dec 24, 2017

View reviewed changes

nicolo-ribaudo pushed a commit to babel/babel that referenced this pull request Nov 14, 2019

Use new scopeManager/visitorKeys APIs (babel/babel-eslint#542)

5d6d52f

(fixes eslint/eslint#9762)

congcongcong250 mentioned this pull request Dec 11, 2019

Class properties resulting in false positive #487

Closed

This was referenced Jun 15, 2021

chore(deps): update dependency babel-eslint to v10 - autoclosed thebigredgeek/apollo-resolvers#70

Closed

Update babel monorepo (major) - abandoned - autoclosed andrejslogins/react-rock-paper-scissors#36

Closed

hzoo mentioned this pull request Jul 13, 2021

Breaking: Switch to ESM (fixes #70) eslint/eslint-scope#71

Merged

renovate bot mentioned this pull request Jul 14, 2021

Update dependency babel-eslint to v10 genenotebook/genenotebook#198

Merged

1 task

renovate bot mentioned this pull request Jul 22, 2021

chore(deps): update babel monorepo (major) - autoclosed scratchfoundation/scratch-audio#168

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: improve parser integrations #542

Update: improve parser integrations #542

mysticatea commented Nov 22, 2017

JamesHenry commented Nov 22, 2017

mysticatea commented Nov 23, 2017

JamesHenry commented Nov 23, 2017

mysticatea commented Nov 23, 2017

JamesHenry commented Nov 24, 2017

mysticatea commented Dec 7, 2017

JamesHenry commented Dec 7, 2017

kaicataldo commented Dec 7, 2017

mysticatea commented Dec 21, 2017

not-an-aardvark commented Dec 24, 2017 •

edited

Loading

not-an-aardvark commented Dec 24, 2017

platinumazure Dec 24, 2017

not-an-aardvark Dec 24, 2017

hzoo commented Dec 24, 2017 •

edited

Loading

mysticatea commented Dec 24, 2017

kaicataldo left a comment

kaicataldo Dec 24, 2017

mysticatea Dec 24, 2017

kaicataldo Dec 24, 2017

kaicataldo Dec 24, 2017

kaicataldo Dec 24, 2017

kaicataldo Dec 24, 2017

mysticatea Dec 24, 2017

Update: improve parser integrations #542

Update: improve parser integrations #542

Conversation

mysticatea commented Nov 22, 2017

JamesHenry commented Nov 22, 2017

mysticatea commented Nov 23, 2017

JamesHenry commented Nov 23, 2017

mysticatea commented Nov 23, 2017

JamesHenry commented Nov 24, 2017

mysticatea commented Dec 7, 2017

JamesHenry commented Dec 7, 2017

kaicataldo commented Dec 7, 2017

mysticatea commented Dec 21, 2017

not-an-aardvark commented Dec 24, 2017 • edited Loading

not-an-aardvark commented Dec 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzoo commented Dec 24, 2017 • edited Loading

mysticatea commented Dec 24, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark commented Dec 24, 2017 •

edited

Loading

hzoo commented Dec 24, 2017 •

edited

Loading