From 636cddfedf293cfbe4148f62966bc404fc8bb637 Mon Sep 17 00:00:00 2001 From: Stefan Buck Date: Tue, 16 May 2023 22:53:02 +0200 Subject: [PATCH] fix: run file update functions sequentially (#128) --- src/create-tree.ts | 125 +++++++++++++------------- src/types.ts | 4 +- test/create-files-in-sequence.test.ts | 68 ++++++++++++++ 3 files changed, 136 insertions(+), 61 deletions(-) create mode 100644 test/create-files-in-sequence.test.ts diff --git a/src/create-tree.ts b/src/create-tree.ts index 75a2d6f..7081c3d 100644 --- a/src/create-tree.ts +++ b/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; diff --git a/src/types.ts b/src/types.ts index 6164fef..21c1514 100644 --- a/src/types.ts +++ b/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; export type State = { octokit: Octokit; diff --git a/test/create-files-in-sequence.test.ts b/test/create-files-in-sequence.test.ts new file mode 100644 index 0000000..2de14ea --- /dev/null +++ b/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); +});