Skip to content
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 only working with npm@3? #5241

Closed
Turbo87 opened this issue Feb 13, 2016 · 17 comments

Comments

Projects
None yet
7 participants
@Turbo87
Copy link

commented Feb 13, 2016

It appears to me that ESLint@2 is only working with npm@3 now. Is that correct?

I was trying to run mocha-eslint with ESLint@2 and npm@2, but it complained that it Cannot find module 'espree' from '/Users/tbn/Code/mocha-eslint/node_modules'. If I use npm@3 instead it works fine.

I've stepped through the code and it looks like ESLint@2 expects espree (or any other parser/package) to be installed next to eslint in the main node_modules folder, while ESLint@1 was apparently okay with espree being nested in node_modules/eslint/node_modules/espree.

Has this been changed on purpose or should it be considered a bug?

In my limited knowledge the fix should be relatively easy by first looking for the package in the main node_modules and if that fails fallback to looking in node_modules/eslint/node_modules instead.

@BYK

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

I had no problem using ESLint@2.0.0 with NPM@2 but I didn't do a clean install. This is with Node v4. It would be great if you can also provide your Node version.

@BYK

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

I've also verified I have node_modules/eslint/node_modules/espree and not node_modules/espree. Does this help?

@Turbo87

This comment has been minimized.

Copy link
Author

commented Feb 13, 2016

@BYK you can try it with https://github.com/Turbo87/mocha-eslint/tree/30b2f3e987cc44fa641969058fa5ab3be6498f6d:

  • nvm use 4 (results in node v4.2.4 (npm v2.14.12) for me)
    • rm -rf node_modules && npm install && npm test -> fails
  • nvm use 5 (results in node v5.4.1 (npm v3.3.12) for me)
    • rm -rf node_modules && npm install && npm test -> success
@BYK

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

@Turbo87 - I can verify that I hit the same problem when I followed your example. Using Node v4.3.0 and NPM v2.11.0

@BYK BYK added bug core accepted and removed triage labels Feb 13, 2016

@Turbo87

This comment has been minimized.

Copy link
Author

commented Feb 13, 2016

good to know. thanks for the fast reply and taking the time to reproduce this!

@BYK

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

Okay, I think I now understand the issue. Since espree is the default parser, you shouldn't be setting the parser value since ESLint expects the parser to be installed in the project folder, not inside eslint folder. Does that make sense?

@nzakas - can you verify me?

@BYK BYK added question and removed accepted labels Feb 13, 2016

@Turbo87

This comment has been minimized.

Copy link
Author

commented Feb 13, 2016

@BYK removing "parser": "espree" from .eslintrc indeed seems to solve the problem. Is there a reason for still doing the lookup if the parser is set to espree?

Turbo87 added a commit to Turbo87/mocha-eslint that referenced this issue Feb 13, 2016

.eslintrc: Use the default parser
Setting the parser explicitly to "espree" is causing ESLint@2 to look it up
in the main node_modules folder, but since this project is not explicitly
installing "espree" via npm it isn't found.

see eslint/eslint#5241 for details.
@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

That's an interesting bug. @Turbo87 we don't make a special case for espree it's just a default, so it doesn't have a separate code-path for espree and handles it as any other parser.

Turbo87 added a commit to Turbo87/mocha-eslint that referenced this issue Feb 13, 2016

.eslintrc: Use the default parser
Setting the parser explicitly to "espree" is causing ESLint@2 to look it up
in the main node_modules folder, but since this project is not explicitly
installing "espree" via npm it isn't found.

see eslint/eslint#5241 for details.

@nzakas nzakas added accepted and removed question labels Feb 13, 2016

@nzakas

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

This is due to the change in how parser path is calculated. When oarser is provided, its path is calculated differently in 2.0.0 than in 1.x. We should probably just remove parser if it's set to "Espree" so the default case is used.

@Turbo87

This comment has been minimized.

Copy link
Author

commented Feb 13, 2016

@nzakas that is what I was suggesting, yes. It might prevent people from using different espree versions though than the one installed for eslint, but I don't think that would be a big issue.

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

@nzakas I can make a PR for this, if you'd like. Is the default parser set in a config file somewhere that we can check against, or should we be checking if parser === 'espree' manually?

EDIT: Sorry, you can ignore my question - found it in the codebase

@platinumazure

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

I'm not sure I agree with ignoring the espree parser specified manually. It could be that someone has monkey-patched it and needs custom behavior (or has pinned it as a dependency).

I think it would be much better to generate a warning when the parser couldn't be found, and if the parser is espree, we could note that the configuration setting should be removed unless the user is trying to use a specific version of espree that they are managing themselves.

kaicataldo added a commit to kaicataldo/eslint that referenced this issue Feb 13, 2016

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

Totally aware more discussion might be needed, but thought I'd take a stab at fixing this :)

@nzakas

This comment has been minimized.

Copy link
Member

commented Feb 14, 2016

@platinumazure that situation doesn't apply. In 1.x, you could not load anything other than the bundled Espree. This change would ensure the 1.x behavior without errors.

Turbo87 added a commit to Turbo87/mocha-eslint that referenced this issue Feb 15, 2016

.eslintrc: Use the default parser
Setting the parser explicitly to "espree" is causing ESLint@2 to look it up
in the main node_modules folder, but since this project is not explicitly
installing "espree" via npm it isn't found.

see eslint/eslint#5241 for details.
@Turbo87

This comment has been minimized.

Copy link
Author

commented Feb 16, 2016

@kaicataldo did you open a PR for that fix commit yet?

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Feb 16, 2016

Yep! #5247

@Turbo87

This comment has been minimized.

Copy link
Author

commented Feb 16, 2016

@kaicataldo ah awesome, was missing the crosslink to here ;)

@nzakas nzakas closed this in 0b1cd19 Feb 16, 2016

nzakas added a commit that referenced this issue Feb 16, 2016

Merge pull request #5247 from kaicataldo/issue5241
Fix: ignore parser option if set to default parser (fixes #5241)

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.