Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add file caching utility function #20

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ module.exports = {
overrides: [
{
files: ['*.ts'],
extends: ['@metamask/eslint-config-typescript'],
extends: [
'@metamask/eslint-config-typescript',
'@metamask/eslint-config-nodejs',
],
},

{
Expand Down
2 changes: 1 addition & 1 deletion jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ module.exports = {
collectCoverage: true,

// An array of glob patterns indicating a set of files for which coverage information should be collected
collectCoverageFrom: ['./src/**/*.ts'],
collectCoverageFrom: ['./src/**/*.ts', '!./src/logging-utils.ts'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this should be covered just via it being imported. Any reason to exclude it?


// The directory where Jest should output its coverage files
coverageDirectory: 'coverage',
Expand Down
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@
"test": "jest && jest-it-up",
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/utils": "^8.2.0"
},
"devDependencies": {
"@lavamoat/allow-scripts": "^2.3.1",
"@lavamoat/preinstall-always-fail": "^1.0.0",
Expand Down
4 changes: 4 additions & 0 deletions src/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/**
* The number of milliseconds in an hour, used as a cache age.
*/
export const ONE_HOUR = 60 * 60 * 1000;
261 changes: 261 additions & 0 deletions src/fetch-or-populate-file-cache.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,261 @@
import { readJsonFile, writeJsonFile } from '@metamask/utils/node';
import path from 'path';

import { fetchOrPopulateFileCache } from './fetch-or-populate-file-cache';
import { withinSandbox, fakeDateOnly } from '../tests/helpers';

describe('fetchOrPopulateFileCache', () => {
beforeEach(() => {
fakeDateOnly();
});

afterEach(() => {
jest.useRealTimers();
});

describe('if the given file does not already exist', () => {
it('saves the return value of the given function in the file as JSON along with its created time', async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:00Z'));

await withinSandbox(async ({ directoryPath: sandboxDirectoryPath }) => {
const filePath = path.join(sandboxDirectoryPath, 'cache');
const data = { foo: 'bar' };

await fetchOrPopulateFileCache({
filePath,
getDataToCache: () => data,
});

const cache = await readJsonFile(filePath);
expect(cache).toStrictEqual({
ctime: '2023-01-01T00:00:00.000Z',
data,
});
});
});

it('returns the data that was cached', async () => {
await withinSandbox(async ({ directoryPath: sandboxDirectoryPath }) => {
const filePath = path.join(sandboxDirectoryPath, 'cache');
const dataToCache = { foo: 'bar' };

const cachedData = await fetchOrPopulateFileCache({
filePath,
getDataToCache: () => dataToCache,
});

expect(cachedData).toStrictEqual(dataToCache);
});
});
});

describe('if the given file already exists', () => {
describe('and no explicit max age is given', () => {
describe('and it was created less than an hour ago', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Have you considered parameterize these tests? We might be able to cut down on the repetition that way. These all look like they're testing the same thing with different inputs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could make the max age not configurable. Not sure how likely it is that we'll want to set that in practice

it('does not overwrite the cache', async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:00Z'));

await withinSandbox(
async ({ directoryPath: sandboxDirectoryPath }) => {
const filePath = path.join(sandboxDirectoryPath, 'cache');
const data = { foo: 'bar' };
await writeJsonFile(filePath, {
ctime: new Date('2023-01-01T00:30:00Z').toISOString(),
data,
});

await fetchOrPopulateFileCache({
filePath,
getDataToCache: () => data,
});

const cache = await readJsonFile(filePath);
expect(cache).toStrictEqual({
ctime: '2023-01-01T00:30:00.000Z',
data,
});
},
);
});

it('returns the data in the file', async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:00Z'));

await withinSandbox(
async ({ directoryPath: sandboxDirectoryPath }) => {
const filePath = path.join(sandboxDirectoryPath, 'cache');
const data = { foo: 'bar' };
await writeJsonFile(filePath, {
ctime: new Date('2023-01-01T00:30:00Z').toISOString(),
data,
});

const cachedData = await fetchOrPopulateFileCache({
filePath,
getDataToCache: () => data,
});

expect(cachedData).toStrictEqual(data);
},
);
});
});

describe('and it was created more than an hour ago', () => {
it('overwrites the cache', async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:00Z'));

await withinSandbox(
async ({ directoryPath: sandboxDirectoryPath }) => {
const filePath = path.join(sandboxDirectoryPath, 'cache');
const dataToCache = { foo: 'bar' };
await writeJsonFile(filePath, {
ctime: new Date('2023-01-01T01:00:01Z').toISOString(),
data: dataToCache,
});

await fetchOrPopulateFileCache({
filePath,
getDataToCache: () => dataToCache,
});

const cache = await readJsonFile(filePath);
expect(cache).toStrictEqual({
ctime: '2023-01-01T01:00:01.000Z',
data: dataToCache,
});
},
);
});

it('returns the data in the file', async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:00Z'));

await withinSandbox(
async ({ directoryPath: sandboxDirectoryPath }) => {
const filePath = path.join(sandboxDirectoryPath, 'cache');
const dataToCache = { foo: 'bar' };
await writeJsonFile(filePath, {
ctime: new Date('2023-01-01T01:00:01Z').toISOString(),
data: dataToCache,
});

const cachedData = await fetchOrPopulateFileCache({
filePath,
getDataToCache: () => dataToCache,
});

expect(cachedData).toStrictEqual(dataToCache);
},
);
});
});
});

describe('and a max age is given', () => {
describe('and it was created less than <max age> seconds ago', () => {
it('does not overwrite the cache', async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:00Z'));

await withinSandbox(
async ({ directoryPath: sandboxDirectoryPath }) => {
const filePath = path.join(sandboxDirectoryPath, 'cache');
const data = { foo: 'bar' };
await writeJsonFile(filePath, {
ctime: new Date('2023-01-01T00:00:04Z').toISOString(),
data,
});

await fetchOrPopulateFileCache({
filePath,
getDataToCache: () => data,
maxAge: 5000,
});

const cache = await readJsonFile(filePath);
expect(cache).toStrictEqual({
ctime: '2023-01-01T00:00:04.000Z',
data,
});
},
);
});

it('returns the data in the file', async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:00Z'));

await withinSandbox(
async ({ directoryPath: sandboxDirectoryPath }) => {
const filePath = path.join(sandboxDirectoryPath, 'cache');
const data = { foo: 'bar' };
await writeJsonFile(filePath, {
ctime: new Date('2023-01-01T00:00:04Z').toISOString(),
data,
});

const cachedData = await fetchOrPopulateFileCache({
filePath,
getDataToCache: () => data,
maxAge: 5000,
});

expect(cachedData).toStrictEqual(data);
},
);
});
});

describe('and it was created more than an hour ago', () => {
it('overwrites the cache', async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:00Z'));

await withinSandbox(
async ({ directoryPath: sandboxDirectoryPath }) => {
const filePath = path.join(sandboxDirectoryPath, 'cache');
const dataToCache = { foo: 'bar' };
await writeJsonFile(filePath, {
ctime: new Date('2023-01-01T00:00:06Z').toISOString(),
data: dataToCache,
});

await fetchOrPopulateFileCache({
filePath,
getDataToCache: () => dataToCache,
maxAge: 5000,
});

const cache = await readJsonFile(filePath);
expect(cache).toStrictEqual({
ctime: '2023-01-01T00:00:06.000Z',
data: dataToCache,
});
},
);
});

it('returns the data in the file', async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:00Z'));

await withinSandbox(
async ({ directoryPath: sandboxDirectoryPath }) => {
const filePath = path.join(sandboxDirectoryPath, 'cache');
const dataToCache = { foo: 'bar' };
await writeJsonFile(filePath, {
ctime: new Date('2023-01-01T00:00:06Z').toISOString(),
data: dataToCache,
});

const cachedData = await fetchOrPopulateFileCache({
filePath,
getDataToCache: () => dataToCache,
maxAge: 5000,
});

expect(cachedData).toStrictEqual(dataToCache);
},
);
});
});
});
});
});
77 changes: 77 additions & 0 deletions src/fetch-or-populate-file-cache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import { fileExists, readJsonFile, writeJsonFile } from '@metamask/utils/node';
import type { Json } from '@metamask/utils/node';

import { ONE_HOUR } from './constants';
import { createModuleLogger, projectLogger } from './logging-utils';

const log = createModuleLogger(projectLogger, 'fetch-or-populate-file-cache');

/**
* The data stored in the cache file.
*/
type FileCache<Data extends Json> = {
/**
* When the data was stored.
*/
ctime: string;
/**
* The cached data.
*/
data: Data;
};

/**
* How long to cache data retrieved from an API (to prevent rate limiting).
*
* Equal to 1 hour.
*/
const DEFAULT_MAX_AGE = ONE_HOUR;

/**
* Avoids rate limits when making requests to an API by consulting a file cache.
*
* Reads the given cache file and returns the data within it if it exists and is
* fresh enough; otherwise runs the given function and saves its return value to
* the file.
*
* @param args - The arguments to this function.
* @param args.filePath - The path to the file where the data should be saved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Perhaps we could update these docs, or the parameter name (or both) to make it more clear that this is the path to the cache file? When I first read this, I thought this was how the data was output (e.g. that this was a "download data and write to this file" function.

* @param args.getDataToCache - A function to get the data that should be cached
* if the cache does not exist or is stale.
* @param args.maxAge - The amount of time (in milliseconds) that the cache is
* considered "fresh". Affects subsequent calls: if `fetchOrPopulateFileCache`
* is called again with the same file path within the duration specified here,
* `getDataToCache` will not get called again, otherwise it will. Defaults to 1
* hour.
*/
export async function fetchOrPopulateFileCache<Data extends Json>({
filePath,
maxAge = DEFAULT_MAX_AGE,
getDataToCache,
}: {
filePath: string;
maxAge?: number;
getDataToCache: () => Data | Promise<Data>;
}): Promise<Data> {
const now = new Date();

if (await fileExists(filePath)) {
const cache = await readJsonFile<FileCache<Data>>(filePath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Have you considered trying to read the file directly and handling the "does not exist" error, rather than checking for existence first? That would align better with what the Node.js docs recommend:

Using fs.exists() to check for the existence of a file before calling fs.open(), fs.readFile(), or fs.writeFile() is not recommended. Doing so introduces a race condition, since other processes may change the file's state between the two calls. Instead, user code should open/read/write the file directly and handle the error raised if the file does not exist.

const createdDate = new Date(cache.ctime);

if (now.getTime() - createdDate.getTime() <= maxAge) {
log(`Reusing fresh cached data under ${filePath}`);
return cache.data;
}
}

log(
`Cache does not exist or is stale; preparing data to write to ${filePath}`,
);
const dataToCache = await getDataToCache();
await writeJsonFile(filePath, {
ctime: now.toISOString(),
data: dataToCache,
});
return dataToCache;
}
9 changes: 0 additions & 9 deletions src/index.test.ts

This file was deleted.

9 changes: 0 additions & 9 deletions src/index.ts

This file was deleted.

Loading