Refactor fixing and linting (#63)

* Major refactor of editor fixing This makes in-editor fixing much faster and fixes a couple bugs * Update changelog * Auto-invoke eslint when config changes, support unsaved files * Don't require a syntax * lint * Deduplication, dispose linter * cleanup
apexskier · Oct 31, 2020 · febcd1a · febcd1a
1 parent 9d34546
commit febcd1a
Show file tree

Hide file tree

Showing 5 changed files with 318 additions and 193 deletions.
diff --git a/ESLint.novaextension/CHANGELOG.md b/ESLint.novaextension/CHANGELOG.md
@@ -1,5 +1,20 @@
 # Changelog
 
+## future
+
+### Added
+
+- Support untitled (new, unsaved) documents
+
+### Changed
+
+- When fixing all issues, apply fixes atomically in editor before saving (make's them must faster)
+
+### Fixed
+
+- Fixed linting failing when extension is first activated
+- Automatically re-lint when preferences change
+
 ## v1.3.0
 
 ### Added

diff --git a/src/linter.ts b/src/linter.ts
@@ -1,6 +1,6 @@
-import type { Linter as EslintLinter } from "eslint";
+import type { ESLint, Linter as ESLintLinter } from "eslint";
 import { eslintOutputToIssue } from "./eslintOutputToIssue";
-import { runEslint } from "./process";
+import { ESLintRunResults, runFixPass, runLintPass } from "./process";
 
 function positionToRange(
   document: TextDocument,
@@ -30,59 +30,91 @@ function positionToRange(
   return new Range(rangeStart, rangeEnd);
 }
 
-export class Linter {
+export class Linter implements Disposable {
   private _issues = new IssueCollection();
   // note - the order of this should match that of _issues
-  private _messages = new Map<
-    string,
-    ReadonlyArray<EslintLinter.LintMessage>
-  >();
+  private _results = new Map<string, ESLint.LintResult>();
   private _processesForPaths: { [path: string]: Disposable | undefined } = {};
 
+  private createResultsHandler(document: TextDocument) {
+    return (output: Error | ESLintRunResults) => {
+      if (output instanceof Error) {
+        throw output;
+      }
+      delete this._processesForPaths[document.uri];
+      if (output.length !== 1) {
+        console.warn(JSON.stringify(output));
+        throw new Error("Unexpected results from linter");
+      }
+      const result = output[0];
+      this._results.set(document.uri, result);
+      this._issues.set(document.uri, result.messages.map(eslintOutputToIssue));
+    };
+  }
+
   lintDocument(document: TextDocument) {
-    if (!document.syntax) {
-      return;
-    }
     const contentRange = new Range(0, document.length);
     const content = document.getTextInRange(contentRange);
-
-    this.lintString(content, document.uri, document.syntax);
+    this._processesForPaths[document.uri]?.dispose();
+    this._processesForPaths[document.uri] = runLintPass(
+      content,
+      document.isUntitled ? null : document.uri,
+      document.syntax,
+      this.createResultsHandler(document)
+    );
   }
 
-  private lintString(string: string, uri: string, syntax: string) {
-    const path = nova.path.normalize(uri);
-    this._processesForPaths[path]?.dispose();
-    this._processesForPaths[path] = runEslint(
-      string,
-      path,
-      syntax,
-      (messages) => {
-        delete this._processesForPaths[path];
-        this._messages.set(path, messages);
-        this._issues.set(path, messages.map(eslintOutputToIssue));
-      }
+  fixDocumentExternal(document: TextDocument) {
+    this._processesForPaths[document.uri]?.dispose();
+    this._processesForPaths[document.uri] = runFixPass(
+      document.uri,
+      document.syntax,
+      this.createResultsHandler(document)
     );
   }
 
+  async fixEditor(editor: TextEditor) {
+    const [messages, issues] = this._getAllMessages(editor.document);
+    const newIssues: Array<Issue> = [];
+    await editor.edit((edit) => {
+      messages
+        .slice()
+        .reverse()
+        .forEach((message, i) => {
+          if (message.fix) {
+            const [start, end] = message.fix.range;
+            const range = new Range(start, end);
+            edit.replace(range, message.fix.text);
+          } else {
+            newIssues.push(issues[i]);
+          }
+        });
+    });
+    this._issues.set(editor.document.uri, newIssues);
+  }
+
   removeIssues(uri: string) {
     const path = nova.path.normalize(uri);
-    this._messages.delete(path);
+    this._results.delete(path);
     this._issues.remove(path);
   }
 
-  getAllMessages(editor: TextEditor) {
-    const path = nova.path.normalize(editor.document.uri);
-    return this._messages.get(path) ?? [];
-  }
-
-  getMessageAtSelection(editor: TextEditor) {
-    const path = nova.path.normalize(editor.document.uri);
-    const messages = this._messages.get(path) ?? [];
-    const issues = this._issues.get(path);
-    if (messages.length != issues.length) {
+  private _getAllMessages(
+    document: TextDocument
+  ): [ReadonlyArray<ESLintLinter.LintMessage>, ReadonlyArray<Issue>] {
+    const result = this._results.get(document.uri);
+    const issues = this._issues.get(document.uri);
+    if (!result || result.messages.length != issues.length) {
       throw new Error("inconsistent data in Linter");
     }
-    const message = messages.find((_, i) => {
+    return [result.messages, issues];
+  }
+
+  getMessageAtSelection(
+    editor: TextEditor
+  ): ESLintLinter.LintMessage | undefined {
+    const [messages, issues] = this._getAllMessages(editor.document);
+    return messages.find((_, i) => {
       // annoyingly, nova doesn't provide a getter for this if col/line is set
       // const issueRange = issues[i].textRange!;
       const issue = issues[i];
@@ -100,6 +132,11 @@ export class Linter {
           issueRange.containsIndex(editor.selectedRange.start))
       );
     });
-    return message;
+  }
+
+  dispose() {
+    for (const p in this._processesForPaths) {
+      this._processesForPaths[p]?.dispose();
+    }
   }
 }
diff --git a/src/main.ts b/src/main.ts
@@ -1,45 +1,40 @@
 import { Linter } from "./linter";
-import { fixEslint } from "./process";
+import { initialize } from "./process";
 import { shouldFixOnSave } from "./shouldFixOnSave";
 import { createSuggestionCommandHandler } from "./suggestionCommand";
 
 const compositeDisposable = new CompositeDisposable();
 
-// eslint-disable-next-line no-unused-vars
-function fix(workspace: Workspace, editor: TextEditor): void;
-// eslint-disable-next-line no-unused-vars
-function fix(editor: TextEditor): void;
-function fix(
-  workspaceOrEditor: Workspace | TextEditor,
-  maybeEditor?: TextEditor
-): void {
-  const editor = TextEditor.isTextEditor(workspaceOrEditor)
-    ? workspaceOrEditor
-    : maybeEditor!;
-  if (editor.document.isDirty) {
-    const listener = editor.onDidSave(() => {
-      listener.dispose();
-      innerFix();
-    });
-    editor.save();
-  } else {
-    innerFix();
-  }
+async function asyncActivate() {
+  await initialize();
 
-  function innerFix() {
-    if (!editor.document.path) {
-      nova.workspace.showErrorMessage("This document is missing a path.");
-      return;
+  const linter = new Linter();
+  compositeDisposable.add(linter);
+
+  // eslint-disable-next-line no-unused-vars
+  async function fix(workspace: Workspace, editor: TextEditor): Promise<void>;
+  // eslint-disable-next-line no-unused-vars
+  async function fix(editor: TextEditor): Promise<void>;
+  async function fix(
+    workspaceOrEditor: Workspace | TextEditor,
+    maybeEditor?: TextEditor
+  ): Promise<void> {
+    const editor = TextEditor.isTextEditor(workspaceOrEditor)
+      ? workspaceOrEditor
+      : maybeEditor!;
+
+    await linter.fixEditor(editor);
+    const p = editor.document.path;
+    // this might not be technically necessary, but will run a fix in an external process, which
+    // will help if linting hasn't processed before this is run
+    if (p) {
+      const d = editor.onDidSave(() => {
+        d.dispose();
+        linter.fixDocumentExternal(editor.document);
+      });
+      editor.save();
     }
-    console.log("Fixing", editor.document.path);
-    fixEslint(editor.document.path);
   }
-}
-
-export function activate() {
-  console.log("activating...");
-
-  const linter = new Linter();
 
   compositeDisposable.add(
     nova.commands.register("apexskier.eslint.command.fix", fix)
@@ -50,6 +45,13 @@ export function activate() {
       createSuggestionCommandHandler(linter)
     )
   );
+  compositeDisposable.add(
+    nova.commands.register("apexskier.eslint.command.lintAllEditors", () => {
+      nova.workspace.textEditors.forEach((editor) => {
+        linter.lintDocument(editor.document);
+      });
+    })
+  );
 
   compositeDisposable.add(nova.workspace.onDidAddTextEditor(watchEditor));
 
@@ -75,18 +77,9 @@ export function activate() {
     const editorDisposable = new CompositeDisposable();
 
     editorDisposable.add(
-      editor.onWillSave((editor) => {
+      editor.onWillSave(async (editor) => {
         if (shouldFixOnSave()) {
-          const listener = editor.onDidSave((editor) => {
-            listener.dispose();
-            if (!editor.document.path) {
-              nova.workspace.showErrorMessage(
-                "This document is missing a path."
-              );
-              return;
-            }
-            nova.commands.invoke("apexskier.eslint.command.fix", editor);
-          });
+          await linter.fixEditor(editor);
         }
         linter.lintDocument(editor.document);
       })
@@ -113,8 +106,24 @@ export function activate() {
       document.onDidChangeSyntax((document) => linter.lintDocument(document))
     );
   }
+}
 
-  console.log("activated");
+export function activate() {
+  console.log("activating...");
+  if (nova.inDevMode()) {
+    const notification = new NotificationRequest("activated");
+    notification.body = "ESLint extension is loading";
+    nova.notifications.add(notification);
+  }
+  return asyncActivate()
+    .catch((err) => {
+      console.error("Failed to activate");
+      console.error(err);
+      nova.workspace.showErrorMessage(err);
+    })
+    .then(() => {
+      console.log("activated");
+    });
 }
 
 export function deactivate() {