updating global linter as fallback #1144

bcnichols3 · 2018-06-20T20:54:12Z

reposting my comment from issue #845

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:

if (localNodeModules && !useGlobalEslint) then locationType = 'advanced specified'; and eslintDir uses a ternary to determine relative or absolute path.
else if (!useGlobalEslint) then locationType = 'local project' and eslintDir is pulled from the modules.
if (!isDirectory(eslintDir)) then locationType = 'global' and eslintDir is resolved to global directory, with respect to OS.
if (isDirectory(eslintDir)) then return the object
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

IanVS · 2018-06-21T21:04:33Z

make it so useGlobalEslint remains as an option to supersede local eslint?

It seems to me that using a global eslint installation in place of a locally-specified one (the current behavior) is a bad idea. It can cause errors when the local config relies on plugins, which in order to use the global installation also must be installed globally. But then what if you are working on two projects, each relying on different versions of a particular plugin? Or what if your version of eslint allows particular behavior but the project's package.json specifies a version which will flag that behavior as an error? Generally, it's sucky for all the reasons that global installations of eslint are discouraged in the first place.

It seems like the only really useful time that folks have said they want to have a global installation is when they're working on small scripts that aren't really part of a "project". Or perhaps when working on eslint itself, when it's useful to npm link to create a global reference which points back to the source you're working on. But in both cases, I believe simply having an option to Fall back to global installation would be sufficient, because a standard local installation won't be found in either situation.

So, what do you think about changing the option entirely, from Force global installation to be used to Allow global installation to be fallen back to if a suitable local installation is not found? We're going to have a breaking release soon anyway, so this is probably the time to do it if so desired. I believe this was also the solution requested and agreed upon in the linked issue #845.

…cal linter found

bcnichols3 · 2018-07-10T16:23:28Z

checkin in @IanVS ... I have the option being used as a fallback as requested. One test failure for some reason; can't quite reason out why! Maybe you know?

skylize · 2018-07-13T22:04:13Z

The eslint provider for Linter
  when setting `disableWhenNoEslintConfig` is false
    it errors when no config file is found

This spec is probably expecting to use the fallback-installation of ESLint, which is no longer accessible. There are probably more tests like that, which will yell at you after you fix this one.

skylize · 2018-07-13T21:34:13Z

spec/worker-helpers-spec.js

    })
+
+    // it('falls back to the packaged eslint when no local or global eslint is found', () => {


Why is there a commented out test?

skylize · 2018-07-13T21:39:29Z

src/worker-helpers.js

@@ -49,35 +49,47 @@ function isDirectory(dirPath) {
 }

 export function findESLintDirectory(modulesDir, config, projectPath) {
+  const { localNodeModules } = config.advanced


👍 Destructuring improves readability of the rest of the code.

skylize · 2018-07-13T21:43:01Z

package.json

@@ -69,7 +69,7 @@
      "title": "Global ESLint",
      "properties": {
        "useGlobalEslint": {
-          "title": "Use global ESLint installation",
+          "title": "Always use global ESLint installation",


Maybe "Enable fallback to global ESLint installation" ?

skylize · 2018-07-13T21:45:06Z

spec/linter-eslint-spec.js

@@ -530,9 +530,9 @@ describe('The eslint provider for Linter', () => {

    it('errors when no config file is found', async () => {
      const messages = await lint(editor)
-      const expected = 'Error while running ESLint: No ESLint configuration found..'
+      const expected = 'Error while running ESLint: ESLint not found, please add a local eslint to this project or ensure the global Node path is set correctly and turned on.'


Let's not bloat the specs with the entire formatted error message. Just match on a small distinctive phrase from the message. (Yeah this problem was already there, but your longer message exaggerates it.)

skylize · 2018-07-13T21:49:35Z

spec/worker-helpers-spec.js

@@ -62,14 +62,25 @@ describe('Worker Helpers', () => {
      expect(foundEslint.type).toEqual('global')
    })

-    it('falls back to the packaged eslint when no local eslint is found', () => {
+    it('falls back to the global eslint when no local eslint is found and global is turned on', () => {


Please check for other "found eslint" type specs like this one, that might not make sense anymore.

skylize · 2018-07-13T21:54:10Z

src/worker-helpers.js

+    eslintDir = Path.isAbsolute(cleanPath(localNodeModules))
+      ? Path.join(cleanPath(localNodeModules), 'eslint')
+      : Path.join(projectPath || '', cleanPath(localNodeModules), 'eslint')
+  } else if (!useGlobalEslint) {


I thought we were going to look in local first? This prevents looking in local at all (unless advanced specified) if global is enabled. 😕

skylize · 2018-07-13T21:58:35Z

src/worker-helpers.js

  }
+
  if (isDirectory(eslintDir)) {


isDirectory is expensive. Save the result to a variable so you can use the result twice.

bcnichols3 added 2 commits June 20, 2018 16:51

updating global linter as fallback

fccb502

using more than one if to pass more tests

c024241

1 test failing, fallback used only when option is turned on and no lo…

ffb3ca6

…cal linter found

skylize suggested changes Jul 13, 2018

View reviewed changes

UziTech added the stale label Oct 27, 2021

UziTech closed this Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updating global linter as fallback #1144

updating global linter as fallback #1144

bcnichols3 commented Jun 20, 2018 •

edited

Loading

IanVS commented Jun 21, 2018

bcnichols3 commented Jul 10, 2018

skylize commented Jul 13, 2018

skylize Jul 13, 2018

skylize Jul 13, 2018 •

edited

Loading

skylize Jul 13, 2018

skylize Jul 13, 2018

skylize Jul 13, 2018

skylize Jul 13, 2018

skylize Jul 13, 2018

		})

		// it('falls back to the packaged eslint when no local or global eslint is found', () => {

updating global linter as fallback #1144

updating global linter as fallback #1144

Conversation

bcnichols3 commented Jun 20, 2018 • edited Loading

IanVS commented Jun 21, 2018

bcnichols3 commented Jul 10, 2018

skylize commented Jul 13, 2018

skylize Jul 13, 2018

Choose a reason for hiding this comment

skylize Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

skylize Jul 13, 2018

Choose a reason for hiding this comment

skylize Jul 13, 2018

Choose a reason for hiding this comment

skylize Jul 13, 2018

Choose a reason for hiding this comment

skylize Jul 13, 2018

Choose a reason for hiding this comment

skylize Jul 13, 2018

Choose a reason for hiding this comment

bcnichols3 commented Jun 20, 2018 •

edited

Loading

skylize Jul 13, 2018 •

edited

Loading