octokit / discussions

discussions and planning for Octokit clients
7 stars 5 forks source link

Get archive link API: return URL or full archive content? #12

Open gr2m opened 6 years ago

gr2m commented 6 years ago

This is a follow up question to https://github.com/octokit/node-github/issues/560#issuecomment-353893464 /cc @paulmelnikow

The documentation of GET /repos/:owner/:repo/:archive_format/:ref says

This method will return a 302 to a URL to download a tarball or zipball archive for a repository

In the best "Best practices for integrators" the documentation recommends to Follow any redirects that the API sends you. It references the HTTP Redirects section where it says for 302 redirects:

Temporary redirection. The request should be repeated verbatim to the URI specified in the Location header field but clients should continue to use the original URI for future requests.

On the other side, API is called "Get archive link", so in this case I wonder if we should diverge from the documentation in Octokit libraries and instead return the archive link, instead of full archive content.

The current implementation in node-github returns the full archive as string

const result = github.repos.getArchiveLink({
  owner: 'octokit',
  repo: 'node-github'
})
// result is full content of `octokit/node-github` master branch tarball

What @paulmelnikow suggests instead

const result = github.repos.getArchiveLink({
  owner: 'octokit',
  repo: 'node-github'
})
// result is full URL to download `octokit/node-github` master branch tarball

Let me know what you think :)

ryangribble commented 6 years ago

Speaking for octokit.net we also automatically follow redirects so our method for this does return a Byte[] byte array of the actual archive content. However we have named the method GetArchive so the behaviour is more expected by consumers.

For node, I think if it's called getArchiveLink then yes it should return the URL only. But you could also just call it getArchive (either instead of or in addition to the getArchiveLink call)

paulmelnikow commented 6 years ago

Though I understand the Github instructions are "follow all redirects," it just doesn't make a lot of sense for something that is intended to return a big blob vs. 1k of JSON.

I certainly understand our perspectives could be different, though wanted to ring in with mine.

Since some repos are small and others are really not, it's not great to put the whole thing into memory. In Node the way to do getArchive would be to return a Stream, or else to accept a path and write the archive there.

Maybe the stream is the way to go. That wouldn't get in the way of following redirects.

gr2m commented 6 years ago

@paulmelnikow I fully agree with you. There are a few incoherences in GitHub’s REST API, this is one of them, and I want to make sure that all Octokits are handling it in the same way, which is why I brought up the point.

For octokit.js, I have to keep browser-compatibility in mind, too, so streams are tough.

I tend towards changing the behavior of getArchiveLink() to just return the link, as that’s how I interpret the intent of the endpoint: https://developer.github.com/v3/repos/contents/#get-archive-link

Once you have the link, you can do whatever you want with it, like creating a stream in Node.js.

Any thoughts from @octokit/rb?

paulmelnikow commented 6 years ago

For octokit.js, I have to keep browser-compatibility in mind, too, so streams are tough.

Makes sense!

Once you have the link, you can do whatever you want with it, like creating a stream in Node.js.

Or following that link in in a browser.