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

Fix: Improve monkeypatching for ESLint v2.3.0 #273

Merged
merged 1 commit into from
Mar 14, 2016

Conversation

mysticatea
Copy link
Member

Since v2.3.0, ESLint came to use keys and childVisitorKeys options instead of extending estraverse. VisitorKeys.
ESLint team has the plan to add the ability which extends VisitorKeys into parser plugins, but it may take a long time.

So this PR improves monkey patching for those options.

@hzoo
Copy link
Member

hzoo commented Mar 10, 2016

Thanks @mysticatea!

I added in

exports.VisitorKeys = t.VISITOR_KEYS;
- is that not necessary anymore?

Also for this PR, I would bump https://github.com/babel/babel-eslint/blob/master/package.json#L36 to 2.3.0 so it will actually run on it.

@mysticatea
Copy link
Member Author

Ouch, I updated https://github.com/babel/babel-eslint/blob/master/package.json#L36 to 2.3.0, then 1 test came to failing.
I'm investigating it.

@hzoo
Copy link
Member

hzoo commented Mar 10, 2016

Ah ok, it might not be due to your change specifically though

@mysticatea mysticatea force-pushed the fix-monkeypatching-for-2.3.0 branch 2 times, most recently from f7436d6 to d2299f4 Compare March 10, 2016 13:32
@mysticatea
Copy link
Member Author

Anyway, I share it.

@mysticatea
Copy link
Member Author

I'm sorry, I don't have enough time to investigate it today (JST)...

@hzoo
Copy link
Member

hzoo commented Mar 10, 2016

Thanks - maybe I can check it out tonight

 1) verify fixes issues with flow types and ObjectPattern:
     Error: Expected 0 message(s), got 4 [{"ruleId":"no-unused-vars","severity":1,"message":"'Foo' is defined but never used","line":3,"column":16,"nodeType":"Identifier","source":"  foo({ bar }: Foo) { bar; }"},{"ruleId":"no-shadow","severity":1,"message":"'Foo' is already declared in the upper scope.","line":3,"column":16,"nodeType":"Identifier","source":"  foo({ bar }: Foo) { bar; }"},{"ruleId":"no-unused-vars","severity":1,"message":"'Foo' is defined but never used","line":4,"column":16,"nodeType":"Identifier","source":"  bar({ foo }: Foo) { foo; }"},{"ruleId":"no-shadow","severity":1,"message":"'Foo' is already declared in the upper scope.","line":4,"column":16,"nodeType":"Identifier","source":"  bar({ foo }: Foo) { foo; }"}]
      at verifyAndAssertMessages (test/non-regression.js:28:11)
      at Context.<anonymous> (test/non-regression.js:1353:5)

Looks like it's scoping issue with flow types - we can still merge since it we have the if checks and shouldn't cause more issues

@hzoo
Copy link
Member

hzoo commented Mar 11, 2016

Reran the tests-

@mysticatea
Copy link
Member Author

@nzakas is faster than me.

  • That test failing is caused by here. This patching is changing traversing properties dynamically. But it cannot be changed dynamically in my way.
  • babel-eslint seems to work with eslint@2.4.0.

So I just remove throwing an error when estraverse-fb has not found.

hzoo added a commit that referenced this pull request Mar 14, 2016
Fix: Remove throwing an error when estraverse-fb has not found.
@hzoo hzoo merged commit 0b473e1 into babel:master Mar 14, 2016
@hzoo hzoo mentioned this pull request Aug 4, 2016
2 tasks
nicolo-ribaudo pushed a commit to babel/babel that referenced this pull request Nov 14, 2019
…atching-for-2.3.0

Fix: Remove throwing an error when estraverse-fb has not found.
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.

2 participants