New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eslint 2.3.0 has a missing dependency of estraverse #5476

Closed
mdouglass opened this Issue Mar 5, 2016 · 33 comments

Comments

Projects
None yet
@mdouglass

What version of ESLint are you using?
2.3.0

What configuration and parser (Espree, Babel-ESLint, etc.) are you using?
babel-eslint

What did you do? Please include the actual source code causing the issue.
Out of the box configuration is throwing an error:

Error: Cannot find module 'estraverse-fb'
    at Function.Module._resolveFilename (module.js:339:15)
    at Function.Module._load (module.js:290:25)
    at Module.require (module.js:367:17)
    at monkeypatch (/project/node_modules/babel-eslint/index.js:59:32)
    at Object.exports.parse (/project/node_modules/babel-eslint/index.js:385:5)
    at parse (/project/node_modules/eslint/lib/eslint.js:578:27)
    at EventEmitter.module.exports.api.verify (/project/node_modules/eslint/lib/eslint.js:716:19)
    at processText (/project/node_modules/eslint/lib/cli-engine.js:187:27)
    at processFile (/project/node_modules/eslint/lib/cli-engine.js:227:18)
    at executeOnFile (/project/node_modules/eslint/lib/cli-engine.js:603:23)

@eslintbot eslintbot added the triage label Mar 5, 2016

@unindented

This comment has been minimized.

Show comment
Hide comment
@unindented

unindented Mar 5, 2016

Experiencing the same issue across all my repos. Greenkeeper is spamming me like crazy.

Experiencing the same issue across all my repos. Greenkeeper is spamming me like crazy.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Mar 5, 2016

Member

This looks like problem with babel-eslint which was relying on estraverse-fb that we removed in this release. @hzoo is that statement correct?

Member

ilyavolodin commented Mar 5, 2016

This looks like problem with babel-eslint which was relying on estraverse-fb that we removed in this release. @hzoo is that statement correct?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 5, 2016

Member

If we do #4640, we might be able to fix this the right way.

I'm guessing this is related to #5443.

Member

nzakas commented Mar 5, 2016

If we do #4640, we might be able to fix this the right way.

I'm guessing this is related to #5443.

@nzakas nzakas added bug core blocked evaluating and removed triage labels Mar 5, 2016

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 5, 2016

Member

Oops, hit Enter too soon. Even without removing estraverse-fb, I think the rest of the changes in #5443 would probably have caused other problems with babel-eslint.

Member

nzakas commented Mar 5, 2016

Oops, hit Enter too soon. Even without removing estraverse-fb, I think the rest of the changes in #5443 would probably have caused other problems with babel-eslint.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 5, 2016

Member

I just looked at babel-eslint and it looks like it's just modifying VisitorKeys from estraverse-fb, so #4640 should work.

Member

nzakas commented Mar 5, 2016

I just looked at babel-eslint and it looks like it's just modifying VisitorKeys from estraverse-fb, so #4640 should work.

@mysticatea

This comment has been minimized.

Show comment
Hide comment
@mysticatea

mysticatea Mar 5, 2016

Member

keys option of estraverse and childVisitorKeys option of escope are merged with estraverse.VisitorKeys inside of them.
So I think the previous way of patching still works.

Member

mysticatea commented Mar 5, 2016

keys option of estraverse and childVisitorKeys option of escope are merged with estraverse.VisitorKeys inside of them.
So I think the previous way of patching still works.

@platinumazure

This comment has been minimized.

Show comment
Hide comment
@platinumazure

platinumazure Mar 5, 2016

Member

Shouldn't babel-eslint just declare the dependency if they need it?

Member

platinumazure commented Mar 5, 2016

Shouldn't babel-eslint just declare the dependency if they need it?

@mysticatea

This comment has been minimized.

Show comment
Hide comment
@mysticatea

mysticatea Mar 5, 2016

Member

@platinumazure Actually babel-eslint doesn't need estraverse-fb. babel-eslint is searching and modifying estraverse-fb which ESLint is using, in order to patch for ES.next and Flow features.

As far as I remember, that patch of estraverse-fb is for eslint@<1.9.0

Member

mysticatea commented Mar 5, 2016

@platinumazure Actually babel-eslint doesn't need estraverse-fb. babel-eslint is searching and modifying estraverse-fb which ESLint is using, in order to patch for ES.next and Flow features.

As far as I remember, that patch of estraverse-fb is for eslint@<1.9.0

@egoroof

This comment has been minimized.

Show comment
Hide comment
@egoroof

egoroof Mar 5, 2016

Have the same problem. Eslint@2.3.0 not working :(

egoroof commented Mar 5, 2016

Have the same problem. Eslint@2.3.0 not working :(

@red2678

This comment has been minimized.

Show comment
Hide comment
@red2678

red2678 Mar 5, 2016

Same here.

I just installed estraverse-fb without using the --save or --save-dev flag, all is good.
Then when this is fixed, I'll just run npm-prune or just uninstall it

red2678 commented Mar 5, 2016

Same here.

I just installed estraverse-fb without using the --save or --save-dev flag, all is good.
Then when this is fixed, I'll just run npm-prune or just uninstall it

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Mar 5, 2016

Member

Yeah, not exactly relying on it - estraverse-fb was only added since eslint had it (and since it was removed, it can also be removed from babel-eslint) - so unless I'm missing something I guess we can just do a patch release and remove it. Can do that tomorrow

Member

hzoo commented Mar 5, 2016

Yeah, not exactly relying on it - estraverse-fb was only added since eslint had it (and since it was removed, it can also be removed from babel-eslint) - so unless I'm missing something I guess we can just do a patch release and remove it. Can do that tomorrow

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Mar 5, 2016

Member

Ok nvm, I guess we'll need to wait on #4640 since you can't override espree's visitorkeys either since it's frozen?

Ah I see #5478

Member

hzoo commented Mar 5, 2016

Ok nvm, I guess we'll need to wait on #4640 since you can't override espree's visitorkeys either since it's frozen?

Ah I see #5478

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 5, 2016

Member

Yeah, let's not replace monkey patching with more monkey patching. :) @ilyavolodin said he'd take a look at #4640 this weekend. Otherwise, I'll take a look on Monday.

Member

nzakas commented Mar 5, 2016

Yeah, let's not replace monkey patching with more monkey patching. :) @ilyavolodin said he'd take a look at #4640 this weekend. Otherwise, I'll take a look on Monday.

@mbrookes mbrookes referenced this issue Mar 5, 2016

Merged

[MenuItem] Add flex property #3597

3 of 3 tasks complete

developit added a commit to developit/preact that referenced this issue Mar 5, 2016

developit added a commit to developit/preact-photon that referenced this issue Mar 5, 2016

@jhabdas

This comment has been minimized.

Show comment
Hide comment
@jhabdas

jhabdas Mar 5, 2016

Curious how this issue could have slipped in. My automated lint test caught this before attempting to submit a PR to my Starter Kit with the updates, as shown here:

screen shot 2016-03-05 at 12 22 41 pm

It may be worthwhile to set-up an example project similar to mine using Greenkeeper to programmatically submit PRs for updates, and leveraging Travis as I have for integration testing, so you can release with more confidence end-users won't be impacted. It requires very little work and could save hundreds of man hours.

Nevertheless, thanks for continuing to improve this awesome project. I cannot stress enough to others the usefulness of ESLint in reducing bugs, and improving code quality and consistency.

jhabdas commented Mar 5, 2016

Curious how this issue could have slipped in. My automated lint test caught this before attempting to submit a PR to my Starter Kit with the updates, as shown here:

screen shot 2016-03-05 at 12 22 41 pm

It may be worthwhile to set-up an example project similar to mine using Greenkeeper to programmatically submit PRs for updates, and leveraging Travis as I have for integration testing, so you can release with more confidence end-users won't be impacted. It requires very little work and could save hundreds of man hours.

Nevertheless, thanks for continuing to improve this awesome project. I cannot stress enough to others the usefulness of ESLint in reducing bugs, and improving code quality and consistency.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Mar 5, 2016

Member

@jhabdas This issue is not with ESLint, it's with babel-eslint relying on the dependency of ESLint. Since we don't run tests for babel-eslint there wasn't much we could've done.

Member

ilyavolodin commented Mar 5, 2016

@jhabdas This issue is not with ESLint, it's with babel-eslint relying on the dependency of ESLint. Since we don't run tests for babel-eslint there wasn't much we could've done.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Mar 5, 2016

Member

Yep ^, It's because there is no API for ESLint/estraverse/escope for babel-eslint to use to modify it and thus the "monkey patching". Could potentially be caught by setting up an integration test for babel-eslint when pushing a PR. Should be helped with @ilyavolodin's #5478 PR to pass in data

Member

hzoo commented Mar 5, 2016

Yep ^, It's because there is no API for ESLint/estraverse/escope for babel-eslint to use to modify it and thus the "monkey patching". Could potentially be caught by setting up an integration test for babel-eslint when pushing a PR. Should be helped with @ilyavolodin's #5478 PR to pass in data

BowlingX added a commit to BowlingX/flexcss that referenced this issue Mar 5, 2016

@jhabdas

This comment has been minimized.

Show comment
Hide comment
@jhabdas

jhabdas Mar 5, 2016

@ilyavolodin Given that logic it sounds like this issue should be closed. Am I missing something?

jhabdas commented Mar 5, 2016

@ilyavolodin Given that logic it sounds like this issue should be closed. Am I missing something?

lukasbuenger pushed a commit to lukasbuenger/immutable-treeutils that referenced this issue Mar 9, 2016

jeffshaver added a commit to jeffshaver/safe-framework that referenced this issue Mar 9, 2016

jeffshaver added a commit to jeffshaver/safe-framework that referenced this issue Mar 9, 2016

jeffshaver added a commit to jeffshaver/safe-app that referenced this issue Mar 9, 2016

elliottsj added a commit to elliottsj/memoize-id that referenced this issue Mar 10, 2016

paulfalgout added a commit to paulfalgout/backbone.marionette that referenced this issue Mar 10, 2016

Fix dependencies and html test runner
Locked eslint per eslint/eslint#5476

Added two babel dependencies for node < 5 so the test runner can find the files in node_modules

thangngoc89 added a commit to thangngoc89/blog that referenced this issue Mar 10, 2016

lijunle added a commit to lijunle/depcheck-es6 that referenced this issue Mar 10, 2016

Pin ESLint to 2.2.0.
ESLint 2.3.0 has a BAD bug: eslint/eslint#5476

mjackson added a commit to mjackson/http-client that referenced this issue Mar 10, 2016

richardkall added a commit to richardkall/react-starter that referenced this issue Mar 10, 2016

Upgrade dependencies
- assets-webpack-plugin 3.4.0
- babel-core 6.7.0
- eslint-plugin-react 4.2.1
- expect-jsx 2.4.0
- stylelint 4.5.1
- stylelint-config-richardkall 1.0.1
- webpack-hot-middleware 2.10.0

Skipping ESLint 2.3.0 for now because of eslint/eslint#5476.
@vtambourine

This comment has been minimized.

Show comment
Hide comment
@vtambourine

vtambourine Mar 10, 2016

I am lucky this bug is already opened and can be found on top of issues list! Looking forward to final solution.

I am lucky this bug is already opened and can be found on top of issues list! Looking forward to final solution.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 10, 2016

Member

I'm locking this issue because we are working hard to get this resolved and every new comment creates a notification that distracts us. :)

Member

nzakas commented Mar 10, 2016

I'm locking this issue because we are working hard to get this resolved and every new comment creates a notification that distracts us. :)

@eslint eslint locked and limited conversation to collaborators Mar 10, 2016

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 10, 2016

Member

estraverse fix has landed, so I'm starting work on integrating that. esrecurse (which escope relies on) isn't yet ready, so won't be able to merge until that's complete.

Member

nzakas commented Mar 10, 2016

estraverse fix has landed, so I'm starting work on integrating that. esrecurse (which escope relies on) isn't yet ready, so won't be able to merge until that's complete.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 11, 2016

Member

Published new version of esrecurse, now working on escope.

Member

nzakas commented Mar 11, 2016

Published new version of esrecurse, now working on escope.

@nzakas

This comment has been minimized.

Show comment
Hide comment

@nzakas nzakas removed the blocked label Mar 11, 2016

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 11, 2016

Member

All dependencies have been updated with new functionality, so removing the "blocked" label.

PR: #5543

Member

nzakas commented Mar 11, 2016

All dependencies have been updated with new functionality, so removing the "blocked" label.

PR: #5543

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 11, 2016

Member

PR has been merged. Release is starting.

Member

nzakas commented Mar 11, 2016

PR has been merged. Release is starting.

@BYK BYK assigned BYK and nzakas and unassigned BYK Mar 13, 2016

@BYK

This comment has been minimized.

Show comment
Hide comment
@BYK

BYK Mar 13, 2016

Member

Do you think we can close this since 2.4.0 is out now?

Member

BYK commented Mar 13, 2016

Do you think we can close this since 2.4.0 is out now?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 13, 2016

Member

Yeah, not sure why this didn't automatically close when the PR was merged.

Member

nzakas commented Mar 13, 2016

Yeah, not sure why this didn't automatically close when the PR was merged.

@nzakas nzakas closed this Mar 13, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.