Refactor API code for GitHub backend #508

Benaiah · 2017-08-02T04:17:48Z

- Summary

Refactor of API.js in the GitHub backend. General improvements to
code style, removal of mutation, function interdependencies, and race
conditions (see the commit message for 732ede1 for more details on
that). Code modified is generally much shorter.

Other refactors I'm working on for this PR:

Refactor updateTree
Refactor editorialWorkflowGit
Replace _ with individual function imports.
~~Use Immutable objects throughout to avoid having to convert to and from them in individual functions.~~
~~Refine request handling (possibly remove meta field from requesterrors if a better solution is found).~~

EDIT: I'm closing the scope of this so we can get it merged. Further refactors can come in later PRs.

- Test plan

Tested manually, and existing tests pass.

- Description for the changelog

Refactor API.js in GitHub backend

- A picture of a cute animal (not mandatory but encouraged)

tortilaman

Great optimizations and modernizations overall. Just curious about use of ternary operators to supply empty arrays & objects as opposed to using an && truthiness check. However you describe those.

tortilaman · 2017-08-02T18:26:51Z

src/backends/github/API.js

      ...headers,
+      ...(this.token ? { Authorization: `token ${ this.token }` } : {}),


is there a reason to include the empty {} when there's no token as opposed to using

...(this.token && { Authorization: `token ${ this.token}` }),

I would think unless something is looking for data based on array position, removing an empty object would be preferable.

I don't think this will work, since ...(undefined) and ...(false) both throw a SymbolError.

Nitpick/point of discussion: I honestly think this is less readable. Prettier and more succinct, but less readable. Thoughts?

Actually I take it back, this whole thing is a massive improvement. Going to abstain from nitpicking in favor of faster 🚢ping.

tortilaman · 2017-08-02T18:26:58Z

src/backends/github/API.js

-    })
+    return this.request('/user/repos').then(repos =>
+      Object.keys(repos).some(
+        key => (repos[key].full_name === this.repo) && repos[key].permissions.push


Ooh. Wasn't aware of some. Very useful stuff.

tortilaman · 2017-08-02T18:27:24Z

src/backends/github/API.js

      const contentType = response.headers.get("Content-Type");
      if (contentType && contentType.match(/json/)) {
-        return this.parseJsonResponse(response);
+        return Promise.all([response, response.json()]);


Awesome moving to promises!

- Removes `parseJsonResponse` - Errors on non-ok responses for text responses as well as JSON responses - Adds an optional `meta` field to `APIError` in `src/valueObjects/errors/APIError.js` - Passes github error responses to `APIErrors` generated by `request` in `API.js` of github backend

Benaiah · 2017-08-02T19:27:18Z

Turns out I'd broken storeMetadata - it's now fixed. I've also added a big refactor of the updateTree function (@tortilaman, if you could review that change as well I'd appreciate it).

tech4him1 · 2017-08-02T20:26:33Z

Looks like Travis tests are failing on 8872d18:

github API › should create PR with correct base branch name when publishing with editorial workflow

Expected received Promise to resolve, instead it rejected to value [TypeError: Object.entries is not a function]

Benaiah · 2017-08-02T21:01:23Z

@tech4him1 thanks - it was working on my machine, so I foolishly assumed it was being polyfilled. Replacing with entries from lodash.

tortilaman

I've never looked into Lodash before, but going through this I'm definitely going to have to use this in the future. Super useful for working with data.

tortilaman · 2017-08-02T21:26:00Z

src/backends/github/API.js

+        ];
+
+        const newItems = Object.entries(fileTree).filter(([filename]) => !added[filename]);
+        const [newFiles, newDirs] = partition(newItems, ([, file]) => this.isFile(file));


Not following the ([, file[) part of this. Other than that this seems like a really nice improvement.

@tortilaman it's destructuring the argument and ignoring the first element. For example:

// these are equivalent const f1 = ([ x, y ]) => y > 0 const f2 = ([, y]) => y > 0 f1(0, 1) // true f2(0, 1) // true f1(1, 0) // false f2(1, 0) // false // you can use multiple commas const fourthElement = ([,,,el]) => el f3([1, 2, 3, 4]) // 4

It allows us to prevent creating a name for something we never use later on. If it's obscure enough it's confusing to most people I can change it.

Benaiah · 2017-08-02T23:35:34Z

@tortilaman Got another big function refactored - editorialWorkflowGit. Lodash is indeed super useful (though I've been playing with Ramda lately and that feels like the next level).

…ecaporg#508.

Benaiah · 2017-08-07T23:24:44Z

Fixed a bug in my updateTree refactor which I uncovered while writing my upcoming GitHub API tests.

…ecaporg#508.

kalinchernev · 2017-08-15T18:10:58Z

I was about to create an issue about several issues related to the API calls to github, but found this pull request. I didn't see any modifications in the tests, so it wasn't very easy to figure if these will be related to any changes already fixed.

I suppose many fixes might be already in via the request refactoring, anyway please test these.

Collection folder setting can break the API request silently

For example, I had a config like this:

collections:
    folder: "content/sample-posts/"

The console yields an error because the following request fails api.github.com/repos/kalinchernev/gatsby-jammin/contents/content/sample-posts//15-08-2017/test.md?ts=1502817755076&ref=master - it has double slash.

This configuration fixes this issue

collections:
    folder: "content/sample-posts"

Possible improvement here could be that at least there is a simple component in the body that gives feedback to the user there is an internal error. And of course having some checks for this config entry to ensure correct request to the github API.

The list of posts (or whatever collection type) does not show existing items

I'm using Gatsby, and structure is

.
├── 2017-08-13-test.md
└── sample-posts
    ├── 01-01-2017
    │   └── index.md
    ├── 01-02-2017
    │   └── index.md
    ├── 01-03-2017
    │   └── index.md
    ├── 01-07-2017-Big_Sample_Post
    │   └── index.md
    ├── 02-03-2017
    │   └── index.md
    ├── 02-04-2017
    │   └── index.md
    ├── 15-08-2017
    │   └── test2.md
    ├── 17-03-2017
    │   └── index.md
    └── 23-03-2017
        └── index.md

None of these are displayed in the list of content, even when content has been created via the workflow.

Repo is here if that might help.

Benaiah · 2017-08-15T21:32:19Z

@kalinchernev the first issue is definitely related to how we construct requests - we should be checking to avoid doubling slashes at the beginning and end of user-supplied strings. The other problem appears to be due to the fact that each of your entries is in its own subfolder - this isn't currently supported by the CMS. You're welcome to make an issue regarding that if you want to discuss it further, but it's a bigger concern than those addressed by this PR, which is mostly focused on code cleanup.

erquhart · 2017-08-17T17:07:17Z

@kalinchernev see #513 for the related improvement.

kalinchernev · 2017-08-17T17:40:19Z

@erquhart thanks, I didn't open an issue as suggested, as I managed by flattening as discussed in the existing issue :)

erquhart

@Benaiah solid work on this, thank you!

One place that we still need to improve is not just readability, but understandability, or clarity. Although this code is much leaner and far more self documenting, it's still very dense. We need to break stuff up and notate.

Because of said density, my review only went so deep, so it would be nice to merge this together with your testing PR. That said, LGTM.

erquhart · 2017-08-22T02:18:17Z

src/backends/github/API.js

      ...headers,
+      ...(this.token ? { Authorization: `token ${ this.token }` } : {}),


Nitpick/point of discussion: I honestly think this is less readable. Prettier and more succinct, but less readable. Thoughts?

erquhart · 2017-08-22T02:19:35Z

src/backends/github/API.js

-    return baseHeader;
-  }
-
-  parseJsonResponse(response) {


Nice fat trimmin'

Update: the Git Gateway backend is now using this as well - maybe worth keeping and refactoring.

erquhart · 2017-08-22T02:22:09Z

src/backends/github/API.js

      ...headers,
+      ...(this.token ? { Authorization: `token ${ this.token }` } : {}),


Actually I take it back, this whole thing is a massive improvement. Going to abstain from nitpicking in favor of faster 🚢ping.

erquhart · 2017-08-22T02:29:52Z

src/backends/github/API.js

+    return files
+      .map(file => ({ ...file, file: true }))
+      .reduce((tree, file) => tree.setIn(file.path.split("/"), file), Map())
+      .toJS();


Does it matter that the original version did not include nested keys that were empty and the new one does?
I.e. .filter(part => part) appears to have been dropped.

erquhart · 2017-08-22T02:37:40Z

src/backends/github/API.js

-    });
+    return Promise.all([fileTreePromise, this.getBranch()])
+      .then(([fileTree, branchData]) => this.updateTree(branchData.commit.sha, "/", fileTree))
+      .then(changeTree => this.commit(options.commitMessage, changeTree));


No need for patch?

Wondering that too, was it moved to a different location?

I'm assuming it was superfluous, but wanted to confirm.

I don't remember removing this intentionally, so it was likely an oversight. Thanks for the catch!

erquhart · 2017-08-22T02:42:45Z

src/backends/github/API.js

+      timeStamp: new Date().toISOString(),
+    };
+
+    return (unpublished


A lot of this really should be broken down and notated for clarity at some point in the future.

Definitely agreed.

Functions refactored: - `storeMetadata` - `composeFileTree` - `persistFiles` - `uploadBlob` These refactors had to be done together, since each the methods refactored had implicit dependencies on mutations made by the others. Specifically, `composeFileTree` and `uploadBlob` had to be called in a specific order to mutate their arguments correctly. This led to a race condition in `persistFiles` that was (fortunately) only affecting debugging. This commit removes the mutation and makes the functions more independent, in addition to making general readability and code length improvements to the affected functions. `composeFileTree` has also had its filtering of uploaded files removed, as that was a concern better handled by the caller.

erquhart

@Benaiah I know this PR is on the back burner atm, but I think we're going to need a lot of source documentation and tests added before we can safely merge. Two reasons:

Succinctness and immutability are huge code improvements, but we can't risk bugs for them (hence the need for tests).
Things are a little easier to read here, but not by much. It isn't a fault in your refactor, it's just that there's too much going on here to rely on self-documenting. Source docs can resolve this, and will also make reviews of this PR much more informed.

erquhart

@Benaiah if you can address the comments, get this rebased, and we get one more approval, we can merge.

erquhart · 2018-01-03T23:10:33Z

src/backends/github/API.js

-    return baseHeader;
-  }
-
-  parseJsonResponse(response) {


Update: the Git Gateway backend is now using this as well - maybe worth keeping and refactoring.

erquhart · 2018-01-03T23:16:10Z

src/backends/github/API.js

        return Promise.all(updates)
-          .then(updates => this.request(`${ this.repoURL }/git/trees`, {
+          .then(resolvedUpdates => this.request(`${ this.repoURL }/git/trees`, {
            method: "POST",
            body: JSON.stringify({ base_tree: sha, tree: updates }),


This still needs to be fixed.

tech4him1 · 2018-01-07T02:10:36Z

src/backends/github/API.js

    })
-    .catch((error) => {
-      throw new APIError(error.message, responseStatus, 'GitHub');
+    .catch(err => [err, null])


@Benaiah You mentioned that:

This should be .catch(err => Promise.reject([err, null)) so the other .catch block catches and destructures it properly. This is my bad - it's the same way in my GH API refactor PR.

That's correct, thanks!

begincalendar · 2018-02-08T00:12:55Z

src/backends/github/API.js

+          ? Object.entries(options.params).map(
+            ([key, val]) => `${ key }=${ encodeURIComponent(val) }`)
+          : [];
+    return `${ this.api_root }${ path }?${ [cacheBuster, ...encodedParams].join("&") }`;


Can we make use of modern API here (to avoid re-inventing the wheel)?
Specifically:

URLSearchParams for the query string

URL for the merging of parts (root, path and query string)

Just a note: URLSearchParams isn't supported in MS Edge <17. Otherwise, I think this is a great idea.

they will be removed when PR decaporg#508 is merged

erquhart · 2018-07-31T21:13:47Z

Closing as stale, leaving branch deletion for @Benaiah.

Benaiah added 2 commits August 1, 2017 20:36

Refactor requestHeaders in API.js of github backend

6cbb72f

Refactor urlFor in API.js of github backend

51038e5

tortilaman reviewed Aug 2, 2017

View reviewed changes

Benaiah added 2 commits August 2, 2017 12:25

Consolidate filename generation in storeMetadata in github backend

ed59376

Benaiah force-pushed the gh-api-refactors branch from 8831eb3 to 6a9f6e6 Compare August 2, 2017 19:26

Benaiah force-pushed the gh-api-refactors branch from 313522f to 8872d18 Compare August 2, 2017 19:46

tortilaman reviewed Aug 2, 2017

View reviewed changes

Benaiah force-pushed the gh-api-refactors branch from 8872d18 to f789f65 Compare August 2, 2017 23:35

Benaiah changed the title ~~(WIP) Refactor API code for GitHub backend~~ Refactor API code for GitHub backend Aug 3, 2017

Benaiah requested a review from erquhart August 3, 2017 02:42

tech4him1 added a commit to tech4him1/netlify-cms that referenced this pull request Aug 5, 2017

Fix authentication/permission checking and integrate other files from d…

0118e08

…ecaporg#508.

Benaiah force-pushed the gh-api-refactors branch from cfe952f to a4c8f8f Compare August 7, 2017 23:23

Benaiah force-pushed the gh-api-refactors branch from a4c8f8f to e3c1f60 Compare August 7, 2017 23:34

tech4him1 added a commit to tech4him1/netlify-cms that referenced this pull request Aug 8, 2017

Fix authentication/permission checking and integrate other files from d…

3c3bf13

…ecaporg#508.

tech4him1 mentioned this pull request Aug 8, 2017

(WIP) Add GitLab Backend #517

Closed

10 tasks

tech4him1 added a commit to tech4him1/netlify-cms that referenced this pull request Aug 11, 2017

Code cleanup from decaporg#508.

ef485f1

tech4him1 added a commit to tech4him1/netlify-cms that referenced this pull request Aug 14, 2017

Code cleanup from decaporg#508.

6a6a97c

tech4him1 added a commit to tech4him1/netlify-cms that referenced this pull request Aug 14, 2017

Code cleanup from decaporg#508.

8eb9172

erquhart approved these changes Aug 22, 2017

View reviewed changes

cbarrett pushed a commit to cbarrett/netlify-cms that referenced this pull request Oct 13, 2017

Code cleanup from decaporg#508.

904ea02

erquhart suggested changes Oct 27, 2017

View reviewed changes

cbarrett pushed a commit to cbarrett/netlify-cms that referenced this pull request Nov 21, 2017

Code cleanup from decaporg#508.

e17ffc3

erquhart suggested changes Jan 3, 2018

View reviewed changes

erquhart added area: extensions/backends kind: chore status: 2-code-review labels Jan 3, 2018

trongthanh pushed a commit to goalifyplus/netlify-cms that referenced this pull request Jan 4, 2018

Code cleanup from decaporg#508.

5b9cf95

erquhart pushed a commit to tech4him1/netlify-cms that referenced this pull request Jan 4, 2018

Code cleanup from decaporg#508.

0305d4c

tech4him1 added a commit to tech4him1/netlify-cms that referenced this pull request Jan 4, 2018

Code cleanup from decaporg#508.

13697db

tech4him1 reviewed Jan 7, 2018

View reviewed changes

begincalendar reviewed Feb 8, 2018

View reviewed changes

Benaiah pushed a commit to Benaiah/netlify-cms that referenced this pull request Apr 5, 2018

Code cleanup from decaporg#508.

76abbba

Benaiah pushed a commit that referenced this pull request Apr 19, 2018

Code cleanup from #508.

455c72b

Benaiah pushed a commit that referenced this pull request Apr 20, 2018

Code cleanup from #508.

a64c072

Benaiah pushed a commit that referenced this pull request May 4, 2018

Code cleanup from #508.

6e45601

Benaiah pushed a commit that referenced this pull request May 7, 2018

Code cleanup from #508.

98793fc

Benaiah pushed a commit that referenced this pull request May 18, 2018

Code cleanup from #508.

afc4861

Benaiah pushed a commit that referenced this pull request May 18, 2018

Code cleanup from #508.

af3e902

Benaiah pushed a commit that referenced this pull request May 22, 2018

Code cleanup from #508.

ce568b7

Benaiah pushed a commit that referenced this pull request May 25, 2018

Code cleanup from #508.

4a8fea2

Benaiah pushed a commit that referenced this pull request May 30, 2018

Code cleanup from #508.

a93ad46

erquhart pushed a commit to erquhart/netlify-cms that referenced this pull request Jun 4, 2018

Code cleanup from decaporg#508.

f3e5d40

Benaiah pushed a commit to Benaiah/netlify-cms that referenced this pull request Jun 8, 2018

Code cleanup from decaporg#508.

220ac59

tech4him1 added a commit to tech4him1/netlify-cms that referenced this pull request Jul 30, 2018

chore: ignore conditional assignments in GitHub backend

79a4c41

they will be removed when PR decaporg#508 is merged

erquhart closed this Jul 31, 2018

tech4him1 added the status: stale label Aug 7, 2018

erezrokah deleted the gh-api-refactors branch February 7, 2021 15:37

		...headers,
		...(this.token ? { Authorization: `token ${ this.token }` } : {}),

Refactor API code for GitHub backend #508

Refactor API code for GitHub backend #508

Conversation

Benaiah commented Aug 2, 2017 • edited Loading

tortilaman left a comment

Choose a reason for hiding this comment

tortilaman Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benaiah commented Aug 2, 2017

tech4him1 commented Aug 2, 2017 • edited Loading

Benaiah commented Aug 2, 2017

tortilaman left a comment

Choose a reason for hiding this comment

tortilaman Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Benaiah Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Benaiah commented Aug 2, 2017 • edited Loading

Benaiah commented Aug 7, 2017

kalinchernev commented Aug 15, 2017

Benaiah commented Aug 15, 2017

erquhart commented Aug 17, 2017

kalinchernev commented Aug 17, 2017

erquhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erquhart left a comment

Choose a reason for hiding this comment

erquhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erquhart commented Jul 31, 2018

Benaiah commented Aug 2, 2017 •

edited

Loading

tortilaman Aug 2, 2017 •

edited

Loading

tech4him1 commented Aug 2, 2017 •

edited

Loading

tortilaman Aug 2, 2017 •

edited

Loading

Benaiah Aug 2, 2017 •

edited

Loading

Benaiah commented Aug 2, 2017 •

edited

Loading