Skip to content

Commit

Permalink
fix: cache GitHub API requests by full URL (#65)
Browse files Browse the repository at this point in the history
Cache GitHub API requests based on the URL fetched and not the passed
in target.

Also extends tests to cover caching for the non-GitHub API. Update
test URLs to use domains reserved for testing [1]. Clear the cache
after every test.

[1] https://www.rfc-editor.org/rfc/rfc2606.html
  • Loading branch information
richardlau authored Aug 27, 2021
1 parent d972bca commit ada7116
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 10 deletions.
8 changes: 4 additions & 4 deletions src/lib/network.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,19 @@ const fetch = async (url) => {
};

const fetchGithub = async (target, token, override = false) => {
// check if we already have the response in cache
if (cache.has(target)) return cache.get(target).data;

const github = 'https://api.github.com';
const url = override ? target : `${github}${target}`;
// check if we already have the response in cache
if (cache.has(url)) return cache.get(url).data;

const options = {
headers: {
Authorization: token ? `token ${token}` : ''
}
};
const response = await axios.get(url, options);
// save response in cache for future usage
cache.set(target, response);
cache.set(url, response);
return response.data;
};

Expand Down
50 changes: 44 additions & 6 deletions test/lib/fetch.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const network = require('../../src/lib/network');
jest.mock('axios');

afterEach(() => {
network.clearCache();
jest.clearAllMocks();
});

Expand Down Expand Up @@ -36,11 +37,10 @@ it('should not prepend the url with github api base when override is true', asyn
};
});

await network.fetchGithub('https://sample.com/test/test', null, true);
network.clearCache();
await network.fetchGithub('https://example.test/test/test', null, true);

expect(axios.get).toHaveBeenCalled();
expect(axios.get).toHaveBeenCalledWith('https://sample.com/test/test', {
expect(axios.get).toHaveBeenCalledWith('https://example.test/test/test', {
headers: {
Authorization: ''
}
Expand All @@ -58,7 +58,6 @@ it('should pass the auth token to the request header', async () => {
const token = 'gh_123456789123456789';

await network.fetchGithub('/test/test', token);
network.clearCache();

expect(axios.get).toHaveBeenCalled();
expect(axios.get).toHaveBeenCalledWith('https://api.github.com/test/test', {
Expand All @@ -76,9 +75,48 @@ it('should use the cache on requests with the same target', async () => {
};
});

const content1 = await network.fetchGithub('https://sample.com/test/test');
const content2 = await network.fetchGithub('https://sample.com/test/test');
const content1 = await network.fetch('https://example.test/test/test');
const content2 = await network.fetch('https://example.test/test/test');

expect(axios.get).toHaveBeenCalledTimes(1);
expect(axios.get).toHaveBeenCalledWith('https://example.test/test/test');
expect(content2).toBe(content1);
});

it('should use the cache on Github API requests with the same target', async () => {
// mocking axios get request
axios.get.mockImplementation(() => {
return {
data: ''
};
});

const content1 = await network.fetchGithub('/repos/nodeshift/npcheck');
const content2 = await network.fetchGithub('/repos/nodeshift/npcheck');

expect(axios.get).toHaveBeenCalledTimes(1);
expect(axios.get).toHaveBeenCalledWith(
'https://api.github.com/repos/nodeshift/npcheck',
{ headers: { 'Authorization': '' }}
);
expect(content2).toBe(content1);
});

it('should share cache between override and non-override Github API requests', async () => {
// mocking axios get request
axios.get.mockImplementation(() => {
return {
data: ''
};
});

const content1 = await network.fetchGithub('https://api.github.com/repos/nodeshift/npcheck', null, true);
const content2 = await network.fetchGithub('/repos/nodeshift/npcheck');

expect(axios.get).toHaveBeenCalledTimes(1);
expect(axios.get).toHaveBeenCalledWith(
'https://api.github.com/repos/nodeshift/npcheck',
{ headers: { 'Authorization': '' }}
);
expect(content2).toBe(content1);
});

0 comments on commit ada7116

Please sign in to comment.