Fix large files failing to load. #1224
Conversation
|
Deploy preview for netlify-cms-www ready! Built with commit 57b9ebd |
|
Deploy preview for cms-demo ready! Built with commit 57b9ebd |
| if (cached) { return cached; } | ||
|
|
||
| if (sha) { | ||
| return this.getBlob(sha); |
There was a problem hiding this comment.
Should we fetch the file by path if the SHA is invalid?
There was a problem hiding this comment.
Using the Contents API as a backup makes sense. I feel like there are edge cases like file name changes where this could cause confusion, but that seems pretty low risk.
There was a problem hiding this comment.
I'll need to go back and check -- I'm assuming that we can just check for a 404?
There was a problem hiding this comment.
Although GitHub doesn't seem to ever do GC -- an invalid blob would be really weird. It would likely mean we were hashing the file differently than GitHub was on the server-side, which shouldn't happen.
| return cache.then((cached) => { | ||
| if (cached) { return cached; } | ||
|
|
||
| if (sha) { |
There was a problem hiding this comment.
@Benaiah I'm assuming if we know the SHA we should fetch by SHA first. Is that a problem at all?
There was a problem hiding this comment.
Unless there's some benefit to using the Contents API, e.g. differences in the response object, fetching by SHA make sense to me.
| const dir = path.split('/').slice(0, -1).join('/'); | ||
| return this.listFiles(dir) | ||
| .then(files => files.find(file => file.path === path)) | ||
| .then(file => this.getBlob(file.sha)); |
There was a problem hiding this comment.
Should the case be handled where file is undefined? It should never happen because there is no pagination.
There was a problem hiding this comment.
Shouldn't happen, agreed.
tech4him1
changed the title
|
How on earth did I completely miss this PR?! |
| if (cached) { return cached; } | ||
|
|
||
| return this.request(`${this.repoURL}/git/blobs/${sha}`, { | ||
| headers: { Accept: "application/vnd.github.VERSION.raw" }, |
There was a problem hiding this comment.
We should be able to drop this header as it's only documented for use with the Contents API (unless you experienced an issue that required it).
There was a problem hiding this comment.
No, it is for use with the blobs API as well, see https://developer.github.com/v3/git/blobs/.
| if (cached) { return cached; } | ||
|
|
||
| if (sha) { | ||
| return this.getBlob(sha); |
There was a problem hiding this comment.
Using the Contents API as a backup makes sense. I feel like there are edge cases like file name changes where this could cause confusion, but that seems pretty low risk.
| return cache.then((cached) => { | ||
| if (cached) { return cached; } | ||
|
|
||
| if (sha) { |
There was a problem hiding this comment.
Unless there's some benefit to using the Contents API, e.g. differences in the response object, fetching by SHA make sense to me.
| const dir = path.split('/').slice(0, -1).join('/'); | ||
| return this.listFiles(dir) | ||
| .then(files => files.find(file => file.path === path)) | ||
| .then(file => this.getBlob(file.sha)); |
There was a problem hiding this comment.
Shouldn't happen, agreed.
- Summary
Fixes #1092. Allows files over 1MB to be fetched from GitHub's API.
- Test plan
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)