Breaking: Replace parse with parseForESLint and refactor codebase #509

JamesHenry · 2017-08-09T18:53:01Z

DO NOT MERGE.

This PR is an initial implementation of the proposed changes coming to ESLint in eslint/eslint#8755.

[Updated] summary of changes

I have removed parse() and its required monkey patching in favour of parseForESLint(). This is a breaking change.
I have added a new parseForESLint() method (ESLint will prefer this over a parse() method if one is available anyway).
I have added a lot of comments and broken out code in separate files wherever possible. This was my first babel-eslint PR and so, as discussed with @hzoo, I wanted to leave the codebase in a state where it was a bit more accessible for the next developer. I am sorry that this makes the PR a little more involved to review, as things have moved around a bit.

index.js has deliberately been made very concise, with most things broken out into dedicated files within a new lib/ directory.

[Updated] Remaining work

There are currently three failing tests related to a linting error being expected, but not found. I will try and find time to dig into these soon.

cc @kaicataldo

kaicataldo · 2017-08-09T18:58:34Z

Really excited to see work being done on this. I haven't had time to dig deeply into this myself, but now that you have, what do you think would be needed in the current proposal to avoid monkeypatching altogether? Looks like it's still necessary.

kaicataldo · 2017-08-09T19:01:56Z

Sorry, I see - it's for the backwards compatibility with ESLint 3.x! @hzoo I was under the impression we were dropping support when we land this - what are your thoughts?

hzoo · 2017-08-09T19:14:43Z

Yeah because we are in the process of Babel 7 which is a major bump it's ok to drop old stuff (like we said with minimum of Node 4, babylon 7, etc)

JamesHenry · 2017-08-09T19:20:29Z

@kaicataldo, just to check, which bits are you referring to regarding ESLint 3.x?

kaicataldo · 2017-08-09T19:24:43Z

Hm, maybe I'm just missing something. When would parse be used intead of parseForESLint? Better question is probably still:

what do you think would be needed in the current proposal to avoid monkeypatching altogether?

Thanks for working on this!

JamesHenry · 2017-08-09T19:25:39Z

Ah cool, yeah you mean in general with that. We're on the same page 👍😁

JamesHenry · 2017-08-09T19:26:41Z

If I understand @hzoo correctly I can look to remove parse from this PR before it lands?

kaicataldo · 2017-08-09T19:30:11Z

Okay, awesome 👍 Yeah, I think we should cut a major release and drop support for ESLint v3.x when we release this. At least for me, in an ideal world, babel-eslint becomes as thin a wrapper as we can make it around Babylon, both for maintainability and predictability's sake.

hzoo · 2017-08-09T19:34:15Z

https://github.com/babel/babel-eslint/releases yeah the last releases i've done under v8.0.0 alpha so major was already the plan

mysticatea

Thank you for the investigating!

I found some lack of .ecmaFeatures, it causes the test failing.

mysticatea · 2017-08-10T02:24:14Z

index.js

+  eslintOptions.allowImportExportEverywhere =
+    parserOptions.allowImportExportEverywhere;
+  if (parserOptions.sourceType === "module") {
+    eslintOptions.globalReturn = false;


eslintOptions.ecmaFeatures.globalReturn

mysticatea · 2017-08-10T02:28:32Z

index.js

+  if (parserOptions.sourceType === "module") {
+    eslintOptions.globalReturn = false;
+  } else {
+    delete eslintOptions.globalReturn;


babel-eslint seems to enable allowReturnOutsideFunction option always, so I guess eslintOptions.ecmaFeatures.globalReturn should be true. Or allowReturnOutsideFunction should follow to eslintOptions.ecmaFeatures.globalReturn.

mysticatea · 2017-08-10T02:29:03Z

index.js

+  function analyzeScope(ast) {
+    var eslintScopeOptions = {
+      ignoreEval: true,
+      impliedStrict: eslintOptions.impliedStrict,


eslintOptions.ecmaFeatures.impliedStrict

mysticatea · 2017-08-10T02:29:26Z

index.js

-    if (node.value && node.value.type === "TypeCastExpression") {
-      visitTypeAnnotation.call(this, node.value);
+    if (eslintOptions.globalReturn !== undefined) {
+      eslintOptions.nodejsScope = eslintOptions.globalReturn;


eslintOptions.ecmaFeatures.globalReturn

FYI, the eslintOptions.ecmaFeatures.globalReturn is set by env: {node: true}:

babel-eslint/test/non-regression.js

Line 17 in 42d0c5b

node: true,

JamesHenry · 2017-08-10T14:30:22Z

Thanks @mysticatea - interestingly I did not actually change the ones you pointed out, they were previously also missing .ecmaFeatures.

My main issue was that I was setting nodeJsScope on eslintOptions instead of eslintScopeOptions.

Now that I have added the missing .ecmaFeatures you pointed out and tidied up a few more things I am left with three failing tests. (All linting assertion-related, no more thrown exceptions)

I have updated the description of the PR to reflect the current state, I am back to using your latest commit on the ESLint PR.

hzoo · 2017-08-10T14:44:12Z

also related: #458, #459 (should of tried to merge these in earlier, cc @zertosh). Maybe can rebase after

JamesHenry · 2017-08-11T10:59:02Z

@hzoo Ah cool, but I am little unclear on what the action items are from those PRs, it doesn't seem like agreements were reached?

@zertosh Would you be available to help me get your ideas into this PR?

kaicataldo · 2017-08-16T15:04:38Z

Would it possible to address those issues separately? We could still hold off on a release if we feel like those are important things for the next major, but this feels like a big enough change for one PR already to me!

not-an-aardvark · 2017-08-16T16:52:50Z

One potential reason for doing everything at the same time: This PR is being used to evaluate the features in eslint/eslint#8755. I think the goal of eslint/eslint#8755 is to allow parsers like babel-eslint to avoid monkeypatching altogether. If eslint/eslint#8755 gets merged, and then it turns out babel-eslint still needs to monkeypatch things due to a concern that wasn't previously noticed, then that would be a problem for ESLint because we would be shipping an insufficient implementation and might need to make breaking changes to it. It would be nice to make sure eslint/eslint#8755 fully works for babel-eslint before we ship it in ESLint.

kaicataldo · 2017-08-16T18:15:34Z

@not-an-aardvark Sounds good to me - wasn't clear to me if the other issues were actually related (the first one doesn't seem to be).

not-an-aardvark · 2017-08-16T18:30:16Z

lib/extend-default-visitor-keys.js

+    BABEL_VISITOR_KEYS
+  );
+  extendedVisitorKeys.MethodDefinition.push("decorators");
+  extendedVisitorKeys.Property.push("decorators");


It looks like this still monkeypatches the default visitor keys from ESLint. Since extendedVisitorKeys is only a shallow clone, the extendedVisitorKeys.MethodDefinition array is still shared with the array from ESLint.

not-an-aardvark · 2017-08-16T18:46:27Z

lib/get-eslint-modules.js

+  var Definition = eslintMod.require("eslint-scope/lib/definition").Definition;
+  var Referencer = eslintMod.require("eslint-scope/lib/referencer");
+  var Traverser = eslintMod.require("eslint/lib/util/traverser");
+  var ScopeManager = eslintMod.require("eslint-scope/lib/scope-manager");


All of these APIs are internal, and are subject to change/be removed in the future. Is it necessary to import the specific modules that are being used by ESLint? Could babel-eslint use more generic modules instead, by depending on eslint-scope and estraverse directly? If not, what APIs would ESLint need to provide in order to make this possible?

Ideally, I think babel-eslint should only use ESLint's public APIs. As currently implemented, this will break if e.g. ESLint stops depending on eslint-scope in the future, or changes how Traverser works internally. The overarching goal here is to fix the recurring issue of "babel-eslint is broken due to an internal change in ESLint", but we can only achieve that if babel-eslint avoids using internal APIs (and ESLint provides sufficient public APIs to make everything work and be stable).

I really like and agreed with that perspective, @not-an-aardvark!

I don't think depending on eslint-scope or estraverse directly is an option, because they need to be the matching versions to the end user's installed ESLint version. We would be leaving ourselves open to issues there if eslint-scope was a dep, but ESLint was a resolved peerDep.

I would like to hear from @mysticatea on this too, of course, because I believe I have implemented this how he originally intended in his ESLint PR, but I may be wrong.

I believe we would want to extend the ESLint PR further to provide APIs to solve the fragility you mention, and as I say I think you've made an excellent point.

I can of course hack away on both sides (ESLint and babel-eslint) to try and come up with my "ideal" API surface, and then report back, but would be good to hear @mysticatea thoughts on what you have said and the implementation thus far.

I don't think depending on eslint-scope or estraverse directly is an option, because they need to be the matching versions to the end user's installed ESLint version.

Could you clarify why this is a requirement? It seems like if babel-eslint is being used and is providing scope information, then it doesn't matter what version of eslint-scope is installed for eslint, because the version of eslint-scope from eslint won't get used anyway.

I think babel-eslint should have eslint-scope in own dependencies of package.json. It can pin the version of eslint-scope until it gets public API to override its behavior.

I think babel-eslint should have own visitor keys instead of it extends ESLint's.

I think about fallback: Traverser.getKeys, we can move the fallback logic from ESLint to eslint-scope, so it's shared with all parsers.

@not-an-aardvark I guess I didn't really have a specific scenario in mind, just a general sense of caution around babel-eslint having to concern itself with two modules instead of one (ESLint and eslint-scope instead of just ESLint).

My concerns may well be unfounded, I think it was just a general sense of "what if the version of eslint-scope which babel-eslint depends is not the one which makes sense for the version of ESLint the user has installed", e.g. the user has a previous major version of ESLint installed, but the eslint-scope is intended to be used with the current major version...

I guess I'm thinking about it like this: eslint-scope is just a general scope analysis library. eslint happens to use eslint-scope for scope analysis by default, but provided that eslint-scope doesn't make any major breaking changes, babel-eslint should be to provide scope analysis however it chooses, including by using a separate version of eslint-scope. So it shouldn't matter if the versions are different.

zertosh · 2017-08-17T03:54:29Z

I'll take a look at this in the morning - looks really cool!

JamesHenry · 2017-08-21T17:48:26Z

Based on the comments up to and including my latest one: #509 (comment)

Would you agree @mysticatea @not-an-aardvark @kaicataldo that it makes sense as a next step for me create a new branch on eslint-scope (and bring that in as a dependency) and look to add APIs which allow us to complete this PR without the need for monkey patching nor requiring internal modules from the user's ESLint?

not-an-aardvark · 2017-08-21T20:10:29Z

Would you agree @mysticatea @not-an-aardvark @kaicataldo that it makes sense as a next step for me create a new branch on eslint-scope (and bring that in as a dependency) and look to add APIs which allow us to complete this PR without the need for monkey patching nor requiring internal modules from the user's ESLint?

I'm not very familiar with the current state of eslint-scope's API. Isn't this possible to do already using the subclass method that you've been using? (If we want to allow subclasses of eslint-scope referencers, then we should probably document how they work in eslint-scope. An alternative would be to use composition instead, which might be possible without requiring eslint-scope to expose new APIs.)

JamesHenry · 2017-08-21T20:41:43Z

Full disclaimer, I have no idea how eslint-scope works right now, which is a great reason for me to work on it 😄

JamesHenry · 2017-08-29T12:06:11Z

@not-an-aardvark @mysticatea Please take another look, I have added a new commit which explores the idea of having eslint-scope as a direct dependency of babel-eslint.

It now does not need to resolve ESLint at all, because I have also branched eslint-scope as a POC here to include the Traverser that @mysticatea had in his PR. This simplifies babel-eslint even further which is great.

You can see the branch here: https://github.com/eslint/eslint-scope/compare/api-for-babel-eslint?expand=1

It also adds a couple more things to the public API surface of eslint-scope, which may need to be discussed. I basically just added everything babel-eslint relies on for now. The eslint-scope update is not yet meant to be PR ready (I have not moved any tests over or anything, just minimal POC)

If we do decide to go this route, we will obviously have to update @mysticatea ESLint PR to bring in the Traverser from eslint-scope rather than declaring it inline.

(I also bumped the babel and babylon related deps to their latest versions)

not-an-aardvark · 2017-08-29T20:12:08Z

Thanks @JamesHenry, that looks pretty good to me (although I'm not too familiar with eslint-scope myself). There was a related proposal in eslint/eslint-scope#25 about bringing traversal into eslint-scope, but a few people had concerns about traversal being conceptually separate from scope analysis. I wonder if it would be better to make Traverser its own module (effectively as a slimmed-down estraverse fork) so that babel-eslint, eslint, and eslint-scope could all rely on it.

JamesHenry · 2017-08-29T20:18:42Z

Yeah, it's a fair enough point, it did occur to me too. Perhaps that needs to be discussed at a TSC meeting as it's yet another library to maintain?

not-an-aardvark · 2017-08-31T08:33:50Z

I'm actually not sure why we're using Traverser in ESLint rather than just depending on estraverse. (There is probably a good reason, but I don't know what it is because I'm not very familiar with estraverse's API.) @mysticatea would know more about that.

Would it be possible for babel-eslint to use estraverse rather than Traverser?

JamesHenry · 2017-08-31T08:47:50Z

We were/are I think, the ESLint PR that this PR is working towards removed it: eslint/eslint#8755

JamesHenry · 2017-12-07T23:19:22Z

Closing in favour of #542

Hacky WIP

ca780e5

JamesHenry mentioned this pull request Aug 9, 2017

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

Merged

mysticatea reviewed Aug 10, 2017

View reviewed changes

JamesHenry added 2 commits August 10, 2017 15:17

Further refactor, update to latest commit on ESLint PR

d6e34c1

Minor improvements

18b3760

JamesHenry changed the title ~~WIP: Add parseForESLint and refactor codebase~~ Breaking: Replace parse with parseForESLint and refactor codebase Aug 10, 2017

kaicataldo added the Major label Aug 10, 2017

Minor improvements

8adcff6

hzoo requested review from zertosh and danez August 16, 2017 04:32

not-an-aardvark reviewed Aug 16, 2017

View reviewed changes

POC: Forked eslint-scope as dependency

64bd565

JamesHenry mentioned this pull request Nov 22, 2017

Update: improve parser integrations #542

Merged

JamesHenry closed this Dec 7, 2017

JamesHenry deleted the eslint-improved-parser-integration branch December 7, 2017 23:19

Breaking: Replace parse with parseForESLint and refactor codebase #509

Breaking: Replace parse with parseForESLint and refactor codebase #509

Conversation

JamesHenry commented Aug 9, 2017 • edited Loading

[Updated] summary of changes

[Updated] Remaining work

kaicataldo commented Aug 9, 2017

kaicataldo commented Aug 9, 2017

hzoo commented Aug 9, 2017

JamesHenry commented Aug 9, 2017

kaicataldo commented Aug 9, 2017 • edited Loading

JamesHenry commented Aug 9, 2017

JamesHenry commented Aug 9, 2017

kaicataldo commented Aug 9, 2017

hzoo commented Aug 9, 2017

mysticatea 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

JamesHenry commented Aug 10, 2017 • edited Loading

hzoo commented Aug 10, 2017 • edited Loading

JamesHenry commented Aug 11, 2017

kaicataldo commented Aug 16, 2017

not-an-aardvark commented Aug 16, 2017

kaicataldo commented Aug 16, 2017

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

JamesHenry Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zertosh commented Aug 17, 2017

JamesHenry commented Aug 21, 2017

not-an-aardvark commented Aug 21, 2017

JamesHenry commented Aug 21, 2017

JamesHenry commented Aug 29, 2017 • edited Loading

not-an-aardvark commented Aug 29, 2017

JamesHenry commented Aug 29, 2017

not-an-aardvark commented Aug 31, 2017

JamesHenry commented Aug 31, 2017 • edited Loading

JamesHenry commented Dec 7, 2017

JamesHenry commented Aug 9, 2017 •

edited

Loading

kaicataldo commented Aug 9, 2017 •

edited

Loading

JamesHenry commented Aug 10, 2017 •

edited

Loading

hzoo commented Aug 10, 2017 •

edited

Loading

JamesHenry Aug 21, 2017 •

edited

Loading

JamesHenry commented Aug 29, 2017 •

edited

Loading

JamesHenry commented Aug 31, 2017 •

edited

Loading