fix: run file update functions sequentially (#128)

src/create-tree.ts

@@ -15,69 +15,74 @@ export async function createTree(
     latestCommitTreeSha,
   } = state;
 
-  const tree = (
-    await Promise.all(
-      Object.keys(changes.files).map(async (path) => {
-        const value = changes.files[path];
-
-        if (value === null) {
-          // Deleting a non-existent file from a tree leads to an "GitRPC::BadObjectState" error,
-          // so we only attempt to delete the file if it exists.
-          try {
-            // https://developer.github.com/v3/repos/contents/#get-contents
-            await octokit.request("HEAD /repos/{owner}/{repo}/contents/:path", {
-              owner: ownerOrFork,
-              repo,
-              ref: latestCommitSha,
-              path,
-            });
-
-            return {
-              path,
-              mode: "100644",
-              sha: null,
-            };
-          } catch (error) {
-            return;
-          }
-        }
-
-        // When passed a function, retrieve the content of the file, pass it
-        // to the function, then return the result
-        if (typeof value === "function") {
-          let result;
-
-          try {
-            const { data: file } = await octokit.request(
-              "GET /repos/{owner}/{repo}/contents/:path",
-              {
-                owner: ownerOrFork,
-                repo,
-                ref: latestCommitSha,
-                path,
-              }
-            );
-
-            result = await value(
-              Object.assign(file, { exists: true }) as UpdateFunctionFile
-            );
-          } catch (error) {
-            // @ts-ignore
-            // istanbul ignore if
-            if (error.status !== 404) throw error;
-
-            // @ts-ignore
-            result = await value({ exists: false });
+  let tree = [];
+
+  for (const path of Object.keys(changes.files)) {
+    const value = changes.files[path];
+
+    if (value === null) {
+      // Deleting a non-existent file from a tree leads to an "GitRPC::BadObjectState" error,
+      // so we only attempt to delete the file if it exists.
+      try {
+        // https://developer.github.com/v3/repos/contents/#get-contents
+        await octokit.request("HEAD /repos/{owner}/{repo}/contents/:path", {
+          owner: ownerOrFork,
+          repo,
+          ref: latestCommitSha,
+          path,
+        });
+
+        tree.push({
+          path,
+          mode: "100644",
+          sha: null,
+        });
+        continue;
+      } catch (error) {
+        continue;
+      }
+    }
+
+    // When passed a function, retrieve the content of the file, pass it
+    // to the function, then return the result
+    if (typeof value === "function") {
+      let result;
+
+      try {
+        const { data: file } = await octokit.request(
+          "GET /repos/{owner}/{repo}/contents/:path",
+          {
+            owner: ownerOrFork,
+            repo,
+            ref: latestCommitSha,
+            path,
           }
+        );
+
+        result = await value(
+          Object.assign(file, { exists: true }) as UpdateFunctionFile
+        );
+      } catch (error) {
+        // @ts-ignore
+        // istanbul ignore if
+        if (error.status !== 404) throw error;
+
+        // @ts-ignore
+        result = await value({ exists: false });
+      }
 
-          if (result === null || typeof result === "undefined") return;
-          return valueToTreeObject(octokit, ownerOrFork, repo, path, result);
-        }
+      if (result === null || typeof result === "undefined") continue;
+      tree.push(
+        await valueToTreeObject(octokit, ownerOrFork, repo, path, result)
+      );
+      continue;
+    }
+
+    tree.push(await valueToTreeObject(octokit, ownerOrFork, repo, path, value));
+    continue;
+  }
 
-        return valueToTreeObject(octokit, ownerOrFork, repo, path, value);
-      })
-    )
-  ).filter(Boolean) as TreeParameter;
+  tree = tree.filter(Boolean) as TreeParameter;
 
   if (tree.length === 0) {
     return null;

src/types.ts

@@ -52,7 +52,9 @@ export type UpdateFunctionFile =
       content: never;
     };
 
-export type UpdateFunction = (file: UpdateFunctionFile) => string | File | null;
+export type UpdateFunction = (
+  file: UpdateFunctionFile
+) => string | File | null | Promise<string | File | null>;
 
 export type State = {
   octokit: Octokit;

test/create-files-in-sequence.test.ts

@@ -0,0 +1,68 @@
+import { Octokit as Core } from "@octokit/core";
+import { RequestError } from "@octokit/request-error";
+
+import { createPullRequest } from "../src";
+import { UpdateFunction } from "../src/types";
+const Octokit = Core.plugin(createPullRequest);
+
+test("file functions are called in sequence", async () => {
+  const fixtures = require("./fixtures/update-readme");
+  const fixturePr = fixtures[fixtures.length - 1].response;
+  const octokit = new Octokit();
+
+  octokit.hook.wrap("request", () => {
+    const currentFixtures = fixtures.shift();
+
+    if (currentFixtures.response.status >= 400) {
+      throw new RequestError("Error", currentFixtures.response.status, {
+        request: currentFixtures.request,
+        headers: currentFixtures.response.headers,
+      });
+    }
+
+    return currentFixtures.response;
+  });
+
+  const fileOneStub = jest.fn();
+  const fileTwoStub = jest.fn();
+
+  const pr = await octokit.createPullRequest({
+    owner: "gr2m",
+    repo: "pull-request-test",
+    title: "One comes, one goes",
+    body: "because",
+    head: "happy-path",
+    changes: {
+      files: {
+        "path/to/file1.txt": async () => {
+          expect(fileOneStub.mock.calls).toHaveLength(0);
+          expect(fileTwoStub.mock.calls).toHaveLength(0);
+
+          fileOneStub();
+
+          // Delay execution till next event loop phase
+          // to ensure file functions are called in sequence
+          await new Promise((resolve) => setTimeout(resolve, 0));
+
+          expect(fileOneStub.mock.calls).toHaveLength(1);
+          expect(fileTwoStub.mock.calls).toHaveLength(0);
+          return "";
+        },
+        "path/to/file2.txt": async () => {
+          expect(fileOneStub.mock.calls).toHaveLength(1);
+          expect(fileTwoStub.mock.calls).toHaveLength(0);
+
+          fileTwoStub();
+
+          expect(fileOneStub.mock.calls).toHaveLength(1);
+          expect(fileTwoStub.mock.calls).toHaveLength(1);
+          return "";
+        },
+      },
+      commit: "uppercase README content",
+    },
+  });
+
+  expect(pr).toStrictEqual(fixturePr);
+  expect(fixtures.length).toEqual(0);
+});