Merge pull request #3 from DaVarga/master

Fixing performance issues caused by exessive use of spawn_child

Author: paleite

src/__fixtures__/diff.ts

@@ -45,3 +45,10 @@ index cb3c131..874b8f9 100644
 @@ -17,2 +16,0 @@ import { a } from "../components/a";
 -import { b } from "../context/b";
 -import { c } from "../context/c";`;
+
+export const filenamesAB = `a/dirty.js
+b/dirty.js
+`;
+
+export const filenamesA = `a/dirty.js
+`

src/__snapshots__/index.test.ts.snap

@@ -62,21 +62,25 @@ Object {
     "processors": Object {
       "diff": Object {
         "postprocess": [Function],
+        "preprocess": [Function],
         "supportsAutofix": true,
       },
       "staged": Object {
         "postprocess": [Function],
+        "preprocess": [Function],
         "supportsAutofix": true,
       },
     },
   },
   "processors": Object {
     "diff": Object {
       "postprocess": [Function],
+      "preprocess": [Function],
       "supportsAutofix": true,
     },
     "staged": Object {
       "postprocess": [Function],
+      "preprocess": [Function],
       "supportsAutofix": true,
     },
   },

src/git.test.ts

@@ -1,7 +1,8 @@
 import * as child_process from "child_process";
 import { mocked } from "ts-jest/utils";
-import { getDiffForFile, getRangesForDiff } from "./git";
-import { hunks, includingOnlyRemovals } from "./__fixtures__/diff";
+import { getDiffFileList, getDiffForFile, getRangesForDiff } from "./git";
+import { hunks, includingOnlyRemovals, filenamesAB } from "./__fixtures__/diff";
+import path from "path";
 
 jest.mock("child_process");
 
@@ -25,4 +26,28 @@ describe("git", () => {
     expect(diffFromFile).toContain("diff --git");
     expect(diffFromFile).toContain("@@");
   });
+
+  it("should hit the cached diff of a file", () => {
+    jest.mock("child_process").resetAllMocks();
+    mockedChildProcess.execSync.mockReturnValue(Buffer.from(hunks));
+
+    const diffFromFileA = getDiffForFile("./mockfileCache.js");
+    const diffFromFileB = getDiffForFile("./mockfileCache.js");
+    expect(mockedChildProcess.execSync).toHaveBeenCalledTimes(1);
+    expect(diffFromFileA).toEqual(diffFromFileB);
+
+    getDiffForFile("./mockfileMiss.js");
+    expect(mockedChildProcess.execSync).toHaveBeenCalledTimes(2);
+  });
+
+  it("should get the list of staged files", () => {
+    mockedChildProcess.execSync.mockReturnValue(Buffer.from(filenamesAB));
+
+    const fileList = getDiffFileList();
+
+    expect(mockedChildProcess.execSync).toHaveBeenCalled();
+    expect(fileList).toEqual(
+      ["a/dirty.js", "b/dirty.js"].map((p) => path.resolve(p))
+    );
+  });
 });

src/git.ts

@@ -5,14 +5,41 @@ import { Range } from "./Range";
 const sanitizeFilePath = (filePath: string) =>
   JSON.stringify(path.resolve(filePath));
 
-const getDiffForFile = (filePath: string, staged = false): string =>
-  child_process
+const diffCacheKey = (filePath: string, staged: boolean): string =>
+  JSON.stringify([path.resolve(filePath), staged]);
+
+const diffCache = new Map<string, string>();
+
+const getDiffForFile = (filePath: string, staged = false): string => {
+  // Get diff entry from cache
+  const cachedDiff = diffCache.get(diffCacheKey(filePath, staged));
+  if (cachedDiff) {
+    // If entry is not falsy return it
+    return cachedDiff;
+  } else {
+    // Otherwise spawn child_process set it in cache and return it
+    const diff = child_process
+      .execSync(
+        `git diff --diff-filter=ACM --unified=0 HEAD ${
+          staged ? "--staged" : ""
+        } -- ${sanitizeFilePath(filePath)}`
+      )
+      .toString();
+    diffCache.set(diffCacheKey(filePath, staged), diff);
+    return diff;
+  }
+};
+
+const getDiffFileList = (staged = false): string[] => {
+  return child_process
     .execSync(
-      `git diff --diff-filter=ACM --unified=0 HEAD ${
-        staged ? " --staged" : ""
-      } -- ${sanitizeFilePath(filePath)}`
+      `git diff --diff-filter=ACM HEAD ${staged ? "--staged" : ""} --name-only`
     )
-    .toString();
+    .toString()
+    .split("\n")
+    .filter((filePath) => filePath.length)
+    .map((filePath) => path.resolve(filePath));
+};
 
 const isHunkHeader = (input: string) => {
   const hunkHeaderRE = new RegExp(/^@@ .* @@/g);
@@ -57,5 +84,5 @@ const getRangesForDiff = (diff: string): Range[] => {
     .filter(removeNullRanges);
 };
 
-export { getDiffForFile, getRangesForDiff };
+export { getDiffForFile, getRangesForDiff, getDiffFileList };
 export type { Range };

src/processors.ts

@@ -1,12 +1,31 @@
-import { getDiffForFile, getRangesForDiff, Range } from "./git";
+import {
+  getDiffForFile,
+  getRangesForDiff,
+  getDiffFileList,
+  Range,
+} from "./git";
 import { Linter } from "eslint";
 
 const STAGED = true;
 
 const isLineWithinRange = (line: number) => (range: Range) =>
   range.isWithinRange(line);
+let fileList: string[];
 
 const diff = {
+  preprocess: (
+    text: string,
+    filename: string
+  ): { text: string; filename: string }[] => {
+    if (
+      (fileList ? fileList : (fileList = getDiffFileList())).includes(filename)
+    ) {
+      return [{ text, filename }];
+    } else {
+      return [];
+    }
+  },
+
   postprocess: (
     messages: Linter.LintMessage[][],
     filename: string
@@ -35,6 +54,20 @@ const diffConfig = {
 };
 
 const staged = {
+  preprocess: (
+    text: string,
+    filename: string
+  ): { text: string; filename: string }[] => {
+    if (
+      (fileList ? fileList : (fileList = getDiffFileList(true))).includes(
+        filename
+      )
+    ) {
+      return [{ text, filename }];
+    } else {
+      return [];
+    }
+  },
   postprocess: (
     messages: Linter.LintMessage[][],
     filename: string