WIP: Merge in esrecurse and estraverse

[corbinu]

This one is going to take a bit of debate I am sure so I wanted to just whip up an idea of what it would look like before proposing it. This is partly a solution to #17 (and may also help with eslint/eslint#7129). I welcome feedback and totally understand if this is not the way everybody wants to go.

Basically I merged in esrecurse, estraverse and their respective tests. I updated them both to eslint standards and Node 4 compatible ES2015. I then stripped out all code that wasn't used either but eslint-scope or eslint (which actually turned out to be quite a lot) and moved the syntax options into their own file.

This would obviously be breaking but I think it gives us quite a bit more flexibility for things like the Typescript parser.

It also removes the need for fallback VisitorKeys to be passed from eslint as I pulled in the eslint visitor function.

I have created an example branch for eslint here: https://github.com/corbinu/eslint/tree/eslint-scope-4
Which removes estravserse as a dep and removes the Travserser class. All tests pass.

However it should be noted that no part of eslint-scope actually depends on the Traverser any more it would make just as much sense to simply move that into eslint itself.

This is just a discussion point so for now I have disabled the JS Doc rule. I look forward to hearing everybody's thoughts

@nzakas @JamesHenry @soda0289 @ilyavolodin

[not-an-aardvark]

Thanks for the PR, this is an interesting idea. Could you clarify the advantages of doing this? Conceptually, it seems like traversal is a separate concern from scope analysis, so it would make sense for them to be separate libraries.

I'm not adamantly opposed to this if there's good reason to do it, but I'm concerned that we would be absorbing a significant maintenance cost by rolling our own traverser if there's a good alternative that uses the existing libraries. I think we should only do an approach like this if we're sure we've considered the other alternatives.

[corbinu]

@not-an-aardvark Well the biggest thing was that we were wanting to add new syntax to traverse. The added benefit in my mind was reduced complexity as it seems like eslint is doing some work to deal with the VisitorKeys and such which has now been eliminated.

As for this not being a traversal lib I would say I am totally fine with folding the traverse part into eslint instead.

As for the complexity of maintaining. It this would remove about 40 lines of code from eslint and the new traverser is only 180 lines (down from 848 is estraverse) So it really shouldn't add much of a burden. Given the lack of updates to estraverse and that this eliminates the VisitorKeys and a dependency from both this project and eslint I would argue is probably less effort than currently.

[ilyavolodin]

I'm also having a conceptual issue with merging traverser into scope analysis tool. While I'm not opposed to it, it doesn't feel right. Would like to hear what @nzakas has to say.

[soda0289]

Thanks for putting this together @corbinu. They reason I suggested merging in esrecurse was the project seemed a little dead and might be difficult to get patches into, especially if they only benefited supporting typescript or babel. But I can't see an immediate need to make patches to esrecurse since we can improve this project to support nodes from typescript or babel.

I think we can benefit from allowing an eslint parser to pass in the Syntax and VisitorKeys objects. This way we can support any node we want. The issue eslint/eslint#7129 recommends modify the exported values of estraverse to handle new node types. This solution will work but it's not the cleanest solution and I think could be imporved by defining an API to set Syntax and Visitor Keys.

Even after we allow VisitorKeys to be customized we still need a solution to scope new node types, maybe with hooks or plugins, and that is the next big step to allow us to remove monkey patching from babel-eslint and support decorators and other typescript features in eslint.

Another thing to keep in mind is the more we change with eslint-escope the more risk there is of breaking babel-eslint since it works by modifying and overriding imported functions and it expects estraverse to exists and be used by escope. We haven't changed to much yet but a change like this would mean some work would have to be done there to restore functionality.

[soda0289]

I have gone thought the esrecurse code and it uses estraverse only for the objects Syntax and VisitorKeys. By including that code here, or forking the project, we could also eliminate the dependency on estraverse since escurse, and eslint-scope, uses none of the actual traverse functionality only the json objects.

After reviewing the PR I noticed that this commit no longer allows for VisitorKeys to be passed in. This is something we still need since we don't want to hardcode those values. Unless another solution is possible.

[corbinu]

@ilyavolodin Do you oppose merging esrecurse here and estraverse into eslint then? Basically replacing the Traverser class with the Traverser one here

@soda0289 We don't need to use the VisitorKeys anymore as just as when eslint uses estraverse this loops over all the options as expect those blacklisted specifically "parent", "leadingComments", "trailingComments"

[ilyavolodin]

@corbinu Honestly, I don't know enough about requirements from TypeScript and Flow to say if that's something that needs to be done. Merging Traverser into ESLint sounds like a better plan, but I'm just not familiar enough with this stuff to make any calls.

[soda0289]

@corbinu Ahh so this allows escope to see every node in the AST. Sorry i missed that part. I will try this out myself.

[corbinu]

@soda0289 Ya that is basically the only purpose to the Traverser class in eslint

[soda0289]

@corbinu Yes I see that now. Why does the issue eslint/eslint#7129 exist. Doesn't eslint already traverse every node in escope and with estraverse?

[soda0289]

I see they don't pass in a childVisitor to escope.anaylzye. Your PR would fix this by removing the need for childVistior

[not-an-aardvark]

I remember looking into this awhile ago when I created eslint/eslint#8232. I think the Traverser class iterates over all keys as a fallback for when estraverse doesn't know what to do with a particular node type. When traversing a node type that estraverse knows about, it keeps the usual estraverse behavior.

[corbinu]

@not-an-aardvark Ok sorry I should have been more clear.. this closes that issue then.

[not-an-aardvark]

No worries, I was confused when I was looking at it too. 😃

[nzakas]

I'd suggest holding off on this. While I understand the idea, I think we are better off creating the first version of ESLint-scope to be as closed to escope as possible to minimize compatibility issues. Once we have a release incorporated into ESLint, we will be in a better position to determine whether something like this makes sense.

Keep in mind that estraverse is used in several places in ESLint, so there's probably some benefit in keeping that as a standalone package.

[corbinu]

@nzakas We already released the first eslint-scope. Is there something you wanted to add? Or are you saying is best to hold off until eslint 5?

The changes for eslint are actually quite minimal: corbinu/eslint@3d34535

How do you feel about me removing the estravsere work and just leaving the esresurse work here. Then opening a PR against eslint to either merge that in there or create a new eslint-travserse package off of it. I am totally fine if you still want to tag that work as being something to look at for eslint 5

[kaicataldo]

Friendly ping - do we want to re-evaluate this, or is it maybe time to close it?

[corbinu]

I still think this makes sense but that’s just my thoughts.

[nzakas]

I'm still not sure how much benefit this really gives us at this point. We've gotten pretty far along this course, and merging in external dependencies means we will have more to maintain. Unless anyone else on the team feels strongly, I don't think we should merge this.

[corbinu]

ok I will just close then