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

Update: improve parser integrations #542

Merged
merged 8 commits into from
Dec 24, 2017

Conversation

mysticatea
Copy link
Member

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
Copy link
Member

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

@mysticatea
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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!

@mysticatea mysticatea changed the title [WIP] Update: improve parser integrations Update: improve parser integrations Dec 7, 2017
@mysticatea
Copy link
Member Author

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

@JamesHenry
Copy link
Member

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
Copy link
Member

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

@mysticatea
Copy link
Member Author

eslint/eslint#8755 has been merged.

@hzoo Could you review this PR?

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Dec 24, 2017

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
Copy link
Member

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.

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

Choose a reason for hiding this comment

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

@mysticatea Should this be edited to ^1.0.0?

Copy link
Member

Choose a reason for hiding this comment

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

@platinumazure I just pushed a commit to update this.

@hzoo
Copy link
Member

hzoo commented Dec 24, 2017

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)

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

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

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


class Referencer extends OriginalReferencer {
// inherits.
visitPattern(node, options, callback) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, just read the comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

nicolo-ribaudo pushed a commit to babel/babel that referenced this pull request Nov 14, 2019
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.

8 participants