refactor: try to avoid futile round-trips to the worker and back

Author: savetheclocktower

lib/config.js

@@ -91,7 +91,7 @@ const Config = {
     if (!configChanged(newConfig, this._currentConfig)) { return; }
     if (this._currentConfig) {
       for (let handler of this.handlers) {
-        handler(newConfig, this._currentConfig || {});
+        handler(newConfig, this._currentConfig);
       }
     }
     this._currentConfig = newConfig;

lib/main.js

@@ -4,6 +4,7 @@ import { CompositeDisposable } from 'atom';
 // eslint-disable-next-line import/no-unresolved
 import { shell } from 'electron';
 import Path from 'path';
+import { diff } from 'deep-object-diff';
 
 import console from './console';
 import Config from './config';
@@ -47,6 +48,7 @@ function eventInvolvesEslintrc (event) {
 export default {
 
   shouldAutoFix (textEditor) {
+    if (this.inactive) { return false; }
     if (textEditor.isModified()) { return false; }
     if (!Config.get('autofix.fixOnSave')) { return false; }
     if (!helpers.hasValidScope(textEditor, this.scopes)) { return false; }
@@ -58,10 +60,21 @@ export default {
   },
 
   async activate () {
-    Config.initialize(atom.project.getPaths()[0]);
+    Config.initialize();
     this.workerPath = Path.join(__dirname, 'worker.js');
     this.jobManager = new JobManager();
 
+    // Inactive is the mode we enter when (a) there is no ESLint for this
+    // project, or (b) there is a too-old ESLint, or (c) there is a v7 ESLint
+    // that we're letting the legacy package handle. We don't bother to send
+    // jobs to the worker in this mode; no use making the round-trip. But we
+    // will wake up if literally anything changes about the project that might
+    // invalidate our assumptions.
+    //
+    // TODO: Consider killing the worker in inactive mode and restarting it
+    // if we leave?
+    this.inactive = false;
+
     // Keep track of whether the user has seen certain notifications. Absent
     // any user-initiated changes, they should see each of these no more than
     // once per session.
@@ -90,13 +103,17 @@ export default {
       ),
 
       // Scan for new .linter-eslint config files when project paths change.
-      atom.project.onDidChangePaths(() => Config.rescan()),
+      atom.project.onDidChangePaths(() => {
+        this.inactive = false;
+        Config.rescan();
+      }),
 
       // React to changes that happen either in a .linter-eslint file or the
       // base package settings.
       Config.onConfigDidChange(
         (config, prevConfig) => {
-          console.debug('Config changed:', config, prevConfig);
+          this.inactive = false;
+          console.debug('Config changed:', config, prevConfig, diff(prevConfig, config));
           if (helpers.configShouldInvalidateWorkerCache(config, prevConfig)) {
             this.clearESLintCache();
           }
@@ -115,6 +132,7 @@ export default {
                 this.jobManager.createWorker();
               })
               .catch(() => {
+                this.inactive = true;
                 this.notifyAboutInvalidNodeBin();
               });
           }
@@ -138,19 +156,13 @@ export default {
           if (event.path.includes('node_modules')) { return false; }
 
           if (eventInvolvesFile(event, '.linter-eslint')) {
+            this.inactive = false;
             Config.rescan();
           }
-          // Any creation, deletion, renaming, or modification of an
-          // `.eslintignore` file anywhere in this project will affect which
-          // files this linter should ignore, and will confuse old instances of
-          // `ESLint` inside the worker script because they seem to cache too
-          // aggressively. So in these cases we've got to clear our cache and
-          // force new `ESLint` instances to be created.
-          if (eventInvolvesFile(event, '.eslintignore')) {
-            this.clearESLintCache();
-          }
-
-          if (eventInvolvesEslintrc(event)) {
+          // Instances of `ESLint` cache whatever configuration details were
+          // present at instantiation time. If any config changes, we can't
+          // re-use old instances.
+          if (eventInvolvesFile(event, '.eslintignore') || eventInvolvesEslintrc(event)) {
             this.clearESLintCache();
           }
         }
@@ -205,13 +217,8 @@ export default {
     this.notified.invalidNodeBin = true;
   },
 
-  // We need to tell the worker to clear its cache when
-  // - any .eslintignore file is changed;
-  // - certain options are changed that must be declared at `ESLint`
-  //   instantiation time.
-  //
   clearESLintCache () {
-    console.warn('Telling the worker to clear its cache!');
+    console.debug('Telling the worker to clear its cache');
     this.jobManager.send({ type: 'clear-cache' });
   },
 
@@ -309,8 +316,8 @@ export default {
     let debugMessage = [
       `Atom version: ${debug.atomVersion}`,
       `linter-eslint-node version: ${debug.packageVersion}`,
-      `Node path: ${debug.nodePath}`,
       `Node version: ${debug.nodeVersion}`,
+      `Node path: ${debug.nodePath}`,
       `ESLint version: ${debug.eslintVersion}`,
       `ESLint location: ${debug.eslintPath}`,
       `Linting in this project performed by: ${whichPackageWillLint}`,
@@ -324,9 +331,19 @@ export default {
       'linter-eslint-node debug information',
       {
         detail: debugMessage.join('\n'),
-        dismissable: true
+        dismissable: true,
+        buttons: [
+          {
+            text: 'Copy',
+            onDidClick () {
+              atom.clipboard.write(debugMessage.join('\n'));
+            }
+          }
+        ]
       }
     );
+
+    return debug;
   },
 
   // Here we're operating entirely outside the purview of the `linter` package.
@@ -343,6 +360,10 @@ export default {
       return;
     }
 
+    // If we're in inactive mode and we get this far, it's because the user
+    // explicitly ran the `Fix Job` command, so we should wake up.
+    this.inactive = false;
+
     if (textEditor.isModified()) {
       atom.notifications.addError(
         `linter-eslint-node: Please save before fixing.`
@@ -391,11 +412,13 @@ export default {
       // this session), we should do nothing here so that an invalid worker
       // behaves as though no linter is present at all.
       this.notifyAboutInvalidNodeBin();
+      this.inactive = true;
       return null;
     }
 
     if (err.type && err.type === 'config-not-found') {
       if (Config.get('disabling.disableWhenNoEslintConfig')) {
+        this.inactive = true;
         return [];
       }
       if (type === 'fix') {
@@ -431,12 +454,14 @@ export default {
 
     if (err.type && err.type === 'no-project') {
       // No project means nowhere to look for an `.eslintrc`.
+      this.inactive = true;
       return null;
     }
 
     if (err.type && err.type === 'version-overlap') {
       // This is an easy one: we don't need to lint this file because
       // `linter-eslint` is present and can handle it. Do nothing.
+      this.inactive = true;
       return null;
     }
 
@@ -449,6 +474,7 @@ export default {
       // this window has been open; no use spamming it on every lint attempt.
       let linterEslintPresent = this.isLegacyPackagePresent();
       let didNotify = this.notified.incompatibleVersion;
+      this.inactive = true;
       if (linterEslintPresent || didNotify || !Config.get('warnAboutOldEslint')) {
         return null;
       } else {
@@ -471,17 +497,19 @@ export default {
           }
         );
         this.notified.incompatibleVersion = true;
+        return null;
       }
-    } else {
-      atom.notifications.addError(
-        `linter-eslint-node Error`,
-        {
-          description: err.error,
-          dismissable: true
-        }
-      );
-      return null;
     }
+
+    // Unknown/unhandled error from the worker.
+    atom.notifications.addError(
+      `linter-eslint-node Error`,
+      {
+        description: err.error,
+        dismissable: true
+      }
+    );
+    return null;
   },
 
   provideLinter () {
@@ -495,6 +523,10 @@ export default {
         if (!atom.workspace.isTextEditor(textEditor)) {
           return null;
         }
+        if (this.inactive) {
+          console.debug('Inactive; skipping lint');
+          return null;
+        }
         const filePath = textEditor.getPath();
         if (!filePath) {
           // Can't report messages back to Linter without a path.

lib/worker.js

@@ -116,14 +116,6 @@ function clearESLintCache () {
   }
 }
 
-function resolveESLint (filePath) {
-  try {
-    return createRequire(filePath).resolve('eslint');
-  } catch (e) {
-    return createRequire(__dirname).resolve('eslint');
-  }
-}
-
 let builtInEslintPath;
 function resolveBuiltInESLint () {
   if (!builtInEslintPath) {
@@ -132,6 +124,14 @@ function resolveBuiltInESLint () {
   return builtInEslintPath;
 }
 
+function resolveESLint (filePath) {
+  try {
+    return createRequire(filePath).resolve('eslint');
+  } catch (e) {
+    return resolveBuiltInESLint();
+  }
+}
+
 function getESLint (filePath, config, { legacyPackagePresent, projectPath }) {
   let { advanced: { useCache } } = config;
   // If two files share a `cwd`, we can reuse any `ESLint` instance that was
@@ -177,7 +177,7 @@ function getESLint (filePath, config, { legacyPackagePresent, projectPath }) {
 
     // Older versions of ESLint won't have this API.
     if (ESLint) {
-      let commonOptions = buildCommonConstructorOptions(config, projectPath || resolveDir);
+      let commonOptions = buildCommonConstructorOptions(config, resolveDir);
 
       const eslintLint = new ESLint({ ...commonOptions, fix: false });
       const eslintFix = new ESLint({ ...commonOptions });