linter-eslint icon indicating copy to clipboard operation
linter-eslint copied to clipboard

Fallback to globally installed eslint if not installed in project

Open gnestor opened this issue 7 years ago • 19 comments

Issue Type

Feature Request

Issue Description

I have a global eslint setup with ~/.eslintrc and globally installed eslint, babel-eslint, etc. This is great because I can now lint all JS projects, not just those that have eslint installed and/or configured. However, if I am working on a project that does have its own eslint setup, then I start running into errors like "eslint-plugin-flowtype not found" because it's required by the project's eslint config but not installed globally.

I propose that this package offer a global eslint installation fallback option so that project installations take precedence.

Thoughts?

gnestor avatar Mar 09 '17 00:03 gnestor

An option like the Use Global ESLint option that already exists? 😛

As a note, from what you are describing the project in question does have an ESLint configuration, so it should be installed in the project.

Arcanemagus avatar Mar 09 '17 17:03 Arcanemagus

@Arcanemagus, actually we don't 'fall-back'. The Use Global ESLint forces the global version to always be used, regardless if the project has its own locally installed ESLint.

We have talked about making this change in the past, and agreed it would be worthwhile. But so far, nobody has done the work and submitted a PR.

IanVS avatar Mar 09 '17 17:03 IanVS

@gnestor I assume you have Use global checked, right? Can you share the output of the Linter Eslint: Debug command?

IanVS avatar Mar 09 '17 17:03 IanVS

@IanVS Currently, when I am working in a project that DOES NOT have eslint set up, I enable Use global. When I switch to a project that DOES have a eslint set up, I need to disable Use global. I am proposing that Use global only uses the global eslint installation if there is no local installation.

My debug:

Atom version: 1.14.4
linter-eslint version: 8.1.3
ESLint version: 3.17.1
Hours since last Atom restart: 14.3
Platform: darwin
Using global ESLint from: /usr/local/lib/node_modules/eslint
Current file's scopes: [
  "source.js.jsx",
  "meta.class.body.js",
  "meta.function.method.js",
  "meta.tag.jsx",
  "JSXAttrs",
  "JSXNested",
  "meta.embedded.expression.js",
  "source.js.jsx",
  "meta.tag.jsx",
  "JSXAttrs",
  "comment.line.double-slash.js"
]
linter-eslint configuration: {
  "disableWhenNoEslintConfig": false,
  "disableWhenNoEslintrcFileInPath": true,
  "lintHtmlFiles": true,
  "useGlobalEslint": true,
  "showRuleIdInMessage": true,
  "eslintrcPath": "",
  "globalNodePath": "",
  "advancedLocalNodeModules": "",
  "eslintRulesDir": "",
  "disableEslintIgnore": false,
  "disableFSCache": false,
  "fixOnSave": false,
  "scopes": [
    "source.js",
    "source.jsx",
    "source.js.jsx",
    "source.babel",
    "source.js-semantic"
  ],
  "rulesToSilenceWhileTyping": [],
  "rulesToDisableWhileFixing": []
}

gnestor avatar Mar 09 '17 18:03 gnestor

Yep, that's what I figured. I totally agree. Are you willing to take a crack at a PR?

IanVS avatar Mar 09 '17 18:03 IanVS

Ya!

gnestor avatar Mar 09 '17 18:03 gnestor

Not sure if I should comment here or start another issue, let me know if I should start another issue. I spent hours yesterday trying to get .eslintrc to be recognized globally. My primary question is, if the .eslintrc Path in Atom doesn't mean that my .eslintrc will be used globally, what does it mean? And is there anything I can do to help with this PR?

What Happened?

I've installed linter-eslint globally using npm. I've also globally installed eslint-config-google using npm. When I try to do Linter eslint: fix file in Atom, I get Notification must be created with string message: as a warning in the top right of Atom. (It doesn't show the rest of the message, if there is any further message.) And it wasn't linting on file change or file save.

What did I try?

There's an option in Atom for .eslintrc Path that says It will only be used when there's no config file in project. That leads me to believe that a global .eslintrc is possible. I set the .eslintrc Path to a variety of directories:

  • /usr/local (which is in my PATH)
  • ~/.atom/packages/linter because someone suggested it, and
  • ~/Desktop

using both objective and relative paths for the last two. And of course .eslintrc was inside each of those when I changed the .eslintrc Path. Changing .eslintrc Path to these 3 different locations still resulted in the above warning and no linting.

Once I put my .eslintrc in the project directory, it linted on file change.

tillydray avatar May 03 '17 12:05 tillydray

@JasonMFry Yours is a separate issue 😉.

To answer your question though, that setting requires the full path to your "global" .eslintrc. For example /home/JasonMFry/lintConfigs/.eslintrc.

Note that ESLint itself already supports a fallback config. If you place a .eslintrc file in your home directory then it will use that if it is unable to find any other configuration for a file. The setting you are talking about only takes effect when both of the following conditions are met:

  • There is no configuration in the project, or any parent directory of the project.
  • There is no configuration in the user's home directory.

Arcanemagus avatar May 04 '17 19:05 Arcanemagus

@gnestor, just checking in, do you still think you'll be able to attempt a PR for this? No pressure, just asking.

IanVS avatar Aug 07 '17 01:08 IanVS

I've been thinking of opening this exact issue, except with one extra feature. I would like to see a fallback that includes a "Using global eslint settings" warning listed as a linter message (line number could be last line of the file). This would encourage using local installations, while still making linter-eslint always available.

I'm willing to stab at a fix also. @gnestor if you've made any progress on this, please link to your working branch on github. Thx

skylize avatar Sep 11 '17 16:09 skylize

@skylize Thank you for stepping up! I've been so busy with work so I haven't had a chance to start this. Let me know I can assist 👍

gnestor avatar Sep 12 '17 15:09 gnestor

Has there been any movement on implementing this feature? Its been several months since the last comment — now that I'm using CRA it's becoming a big annoyance for me.

bcnichols3 avatar Jun 20 '18 16:06 bcnichols3

This is still open to anyone who has the time and willingness to implement. Would you like to take a crack at it, @bcnichols3?

IanVS avatar Jun 20 '18 16:06 IanVS

Hold my beer.

bcnichols3 avatar Jun 20 '18 16:06 bcnichols3

quick question: in findESLintDirectory what is the else statement for? If I'm moving global to the base case, as opposed to the first case (which is where it is now) it'll need an actual statement.

... } else { locationType = 'advanced specified'; eslintDir = Path.join(projectPath || '', cleanPath(config.advanced.localNodeModules), 'eslint') }

bcnichols3 avatar Jun 20 '18 16:06 bcnichols3

The last two blocks (with locationType = 'advanced specified') both have to do with config.advanced.localNodeModules, and whether an absolute or relative path has been specified in the config.

Since I'm not a huge fan of negative if checks, I'd recommend inverting that a bit, and checking for the presence of config.advanced.localNodeModules, then inside that block checking whether it is absolute or not.

So, I envision what you'll have to do is:

  1. first check config.advanced.localNodeModules and whether it is absolute or relative

else:

  1. build a local path to test and call isDirectory on it, and if that succeeds then return that path

but if it fails:

  1. check for config.global.useGlobalEslint and either return that or throw an error if it isn't found.

Does that make any sense?

IanVS avatar Jun 20 '18 16:06 IanVS

Definitely. My original thinking is to make it so useGlobalEslint remains as an option to supersede local eslint? Though that sounds like a terrible idea. I wonder if that check should be removed entirely, so as to not make it easy to jump over a local eslint — which is definitely a bad idea.

Currently I have:

  1. if (localNodeModules && !useGlobalEslint) then locationType = 'advanced specified'; and eslintDir uses a ternary to determine relative or absolute path.
  2. else if (!useGlobalEslint) then locationType = 'local project' and eslintDir is pulled from the modules.
  3. if (!isDirectory(eslintDir)) then locationType = 'global' and eslintDir is resolved to global directory, with respect to OS.
  4. if (isDirectory(eslintDir)) then return the object
  5. else then `throw new Error('ESLint not found, please add a local eslint to this project or ensure the global Node path is set correctly.')

But I can change that to reflect the thinking above. I also altered worker helper tests accordingly, but two eslint provider tests are failing:

  • when a file is specified in an eslintIgnore key in package.json it will not give warnings when linting the file is throwing the wrong error
  • it errors when no config file is found is also throwing the wrong error

bcnichols3 avatar Jun 20 '18 20:06 bcnichols3

Do you mind opening a PR with what you have so far? It's easier to talk alongside the code I think. Thanks!

IanVS avatar Jun 20 '18 20:06 IanVS

any thoughts @IanVS ?

bcnichols3 avatar Jun 21 '18 15:06 bcnichols3