Repository navigation
Unit test needed to prevent #14 regression #45
Description
Hi there
Awesome library. I have one question though:
It seems like you're appending random query strings to the end of every API call, probably in order to make sure you get a non-cached response on every call.
However, the Gihub API implements a pretty smart HTTP caching, that is being ignored with this library. For people using it, it means that the rate-limit will get used much faster, and apps will try to reload content that is not updated.
Here's an example of what I mean:
/* Using JQuery
____________________________________________ */
$.ajax({
url:"https://user:pass@api.github.com/repos/user/repo",
success: function(data) { console.log(data) }
});
// --> response 200 with content
$.ajax({
url:"https://user:pass@api.github.com/repos/user/repo",
success: function(data) { console.log(data) }
});
// --> if called again within 60 seconds this call will never be made,
// as the Github API has HTTP cache of 60 seconds. If after
// 60 seconds and data is not updated, it returns an empty
// 304 not modified, which is much faster
/* Using github.js
____________________________________________ */
var github = new Github({
username: "user",
password: "pass",
auth: "basic"
});
var repo = github.getRepo(user, repo);
repo.show(function(err, repo) { console.log(repo) });
// --> response 200 with content
repo.show(function(err, repo) { console.log(repo) });
// --> response 200 with content, no caching
repo.show(function(err, repo) { console.log(repo) });
// --> response 200 with content, no cachingI saw that you have "implemented smart caching" in a commit, so I'm not sure exactly what's going on. How is this library caching stuff?
Activity
w35l3y commentedon May 21, 2014
👍
I miss it too for the same reason
aendra-rininsland commentedon Oct 17, 2015
Seriously? Lulz @ "smart caching"...
This should be fixed and the whole random number thing removed. Marking as P2 bug for 0.10.7.
[-]Disables HTTP Caching[/-][+]Fix HTTP Caching[/+]aendra-rininsland commentedon Oct 19, 2015
I've removed the cache buster from _request. This means that it should fall back to XMLHttpRequest's caching layer.
Edit: Apparently XMLHttpRequest only caches responses in the browser. This probably needs to be implemented in the lib for NodeJS. Reopening...
aendra-rininsland commentedon Oct 19, 2015
Actually, this is now resolved — GitHub only sends a 304 and no body if you send a conditional header. Therefore there's no reason to add a cache buster to the URL (other than to get around the browser's caching; if anything, the cache buster should be added via an option, though given I am currently unsure how to do that, am leaving it be for the moment).
The question is, how best to deal with said-headers? I really, really don't feel like writing some caching noise right now, so am closing this and reopening #53. Please direct all conversation re: conditional headers and caching there. Thanks!
w35l3y commentedon Oct 20, 2015
Just to mention
getTimewas proposed by the author to fix #14I haven't tested it to verify whether removing it would reopen #14
aendra-rininsland commentedon Oct 21, 2015
@w35l3y Yeah, I was considering that. What's really needed is a unit test for #14. Reopening, renaming, adding label.
[-]Fix HTTP Caching[/-][+]Unit test needed to prevent #14 regression[/+]aendra-rininsland commentedon Oct 21, 2015
Hm. I've added a new unit test, but I don't know whether it will actually test what I'm wanting simply because wrapping the tests in Browserify will cause it to use
node-xmlhttprequestwhen we're wanting to test it using the browser's version of XHR. This might have to be delayed until we migrate the test architecture to Karma or something.AurelioDeRosa commentedon Oct 22, 2015
I'd say that since this is a library mainly used client-side, it's fine for now to have a sure fix for browsers. This should be enough until we have a good plan for the new, shiny, improved version.
Added unit test for github-tools#14, exposed XHR on repo.deleteRef, r…
Added unit test for #14, exposed XHR on repo.deleteRef, reintroduced …
aendra-rininsland commentedon Oct 23, 2015
I've made it so the library now adds the timestamp to the URL if being used in the browser, and have written a unit test that replicates #14. It won't ever fail if used via Tape because of no caching in NodeJS, but can act as a good regression test once we move to Mocha or whatevs.