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

feat: cache latest fuels version #3186

Closed
wants to merge 15 commits into from
5 changes: 5 additions & 0 deletions .changeset/kind-chairs-move.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"fuels": patch
---

feat: cache latest `fuels` version
4 changes: 4 additions & 0 deletions packages/fuels/src/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Command } from 'commander';

import * as cliMod from './cli';
import { Commands } from './cli/types';
import * as checkForUpdatesMod from './cli/utils/checkForAndDisplayUpdates';
import * as loggingMod from './cli/utils/logger';
import { run } from './run';

Expand Down Expand Up @@ -77,6 +78,9 @@ describe('cli.js', () => {
.mockReturnValue(Promise.resolve(command));

const configureCli = vi.spyOn(cliMod, 'configureCli').mockImplementation(() => new Command());
vi.spyOn(checkForUpdatesMod, 'checkForAndDisplayUpdates').mockImplementation(() =>
Promise.resolve()
);
Dhaiwat10 marked this conversation as resolved.
Show resolved Hide resolved

await run([]);

Expand Down
5 changes: 5 additions & 0 deletions packages/fuels/src/cli/config/loadConfig.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { readFileSync } from 'fs';
import { resolve } from 'path';

import { mockCheckForUpdates } from '../../../test/utils/mockCheckForUpdates';
import {
runInit,
bootstrapProject,
Expand All @@ -19,6 +20,10 @@ import { loadConfig } from './loadConfig';
describe('loadConfig', () => {
const paths = bootstrapProject(__filename);

beforeEach(() => {
mockCheckForUpdates();
});

afterEach(() => {
resetConfigAndMocks(paths.fuelsConfigPath);
});
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this tests needs to be re-synced

image

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as versionsMod from '@fuel-ts/versions';

import * as checkForAndDisplayUpdatesMod from './checkForAndDisplayUpdates';
import * as getLatestFuelsVersionMod from './getLatestFuelsVersion';
import * as loggerMod from './logger';

/**
Expand All @@ -17,7 +18,7 @@ describe('checkForAndDisplayUpdates', () => {

const mockDeps = (params: { latestVersion: string; userVersion: string }) => {
const { latestVersion, userVersion } = params;
vi.spyOn(Promise, 'race').mockReturnValue(Promise.resolve(latestVersion));
vi.spyOn(getLatestFuelsVersionMod, 'getLatestFuelsVersion').mockResolvedValue(latestVersion);

vi.spyOn(versionsMod, 'versions', 'get').mockReturnValue({
FUELS: userVersion,
Expand Down
14 changes: 2 additions & 12 deletions packages/fuels/src/cli/utils/checkForAndDisplayUpdates.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,13 @@
import { versions, gt, eq } from '@fuel-ts/versions';

import { getLatestFuelsVersion } from './getLatestFuelsVersion';
import { warn, log } from './logger';

export const getLatestFuelsVersion = async () => {
const response = await fetch('https://registry.npmjs.org/fuels/latest');
const data = await response.json();
return data.version as string;
};

export const checkForAndDisplayUpdates = async () => {
try {
const { FUELS: userFuelsVersion } = versions;

const latestFuelsVersion = await Promise.race<string | undefined>([
new Promise((resolve) => {
setTimeout(resolve, 3000);
}),
getLatestFuelsVersion(),
]);
const latestFuelsVersion = await getLatestFuelsVersion();

if (!latestFuelsVersion) {
log(`\n Unable to fetch latest fuels version. Skipping...\n`);
Expand Down
23 changes: 23 additions & 0 deletions packages/fuels/src/cli/utils/fuelsVersionCache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import fs from 'fs';
import path from 'path';

export const FUELS_VERSION_CACHE_FILE = path.join(__dirname, 'FUELS_VERSION');

export type FuelsVersionCache = string;
petertonysmith94 marked this conversation as resolved.
Show resolved Hide resolved

export const saveToCache = (cache: FuelsVersionCache) => {
fs.writeFileSync(FUELS_VERSION_CACHE_FILE, cache, 'utf-8');
};

export const FUELS_VERSION_CACHE_TTL = 6 * 60 * 60 * 1000; // 6 hours in milliseconds

export const checkAndLoadCache = (): FuelsVersionCache | null => {
if (fs.existsSync(FUELS_VERSION_CACHE_FILE)) {
const cachedVersion = fs.readFileSync(FUELS_VERSION_CACHE_FILE, 'utf-8');
Copy link
Member

Choose a reason for hiding this comment

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

Should we trim this?

Suggested change
const cachedVersion = fs.readFileSync(FUELS_VERSION_CACHE_FILE, 'utf-8');
const cachedVersion = fs.readFileSync(FUELS_VERSION_CACHE_FILE, 'utf-8').trim();

const { mtimeMs: cacheTimestamp } = fs.statSync(FUELS_VERSION_CACHE_FILE);
if (cachedVersion && Date.now() - cacheTimestamp < FUELS_VERSION_CACHE_TTL) {
return cachedVersion;
}
}
Dhaiwat10 marked this conversation as resolved.
Show resolved Hide resolved
return null;
};
50 changes: 50 additions & 0 deletions packages/fuels/src/cli/utils/getLatestFuelsVersion.test.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

What is with these errors?

Screenshot 2024-09-25 at 06 54 01

Copy link
Member Author

Choose a reason for hiding this comment

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

Weird, they pass locally. Investigating

Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import * as cacheMod from './fuelsVersionCache';
import { getLatestFuelsVersion } from './getLatestFuelsVersion';

/**
* @group node
*/
describe('getLatestFuelsVersion', () => {
beforeEach(() => {
vi.resetAllMocks();
});

afterEach(() => {
vi.restoreAllMocks();
});

it('should fail if fetch fails', async () => {
vi.spyOn(global, 'fetch').mockImplementation(() =>
Promise.reject(new Error('Failed to fetch'))
);
await expect(getLatestFuelsVersion()).rejects.toThrowError('Failed to fetch');
});

it('should throw if fetch times out', async () => {
vi.spyOn(global, 'fetch').mockImplementation(
() =>
new Promise((resolve) => {
setTimeout(resolve, 5000);
})
);
await expect(getLatestFuelsVersion()).rejects.toThrow();
});

it('should return cached version if it exists', async () => {
const cachedVersion = '1.0.0';
vi.spyOn(cacheMod, 'checkAndLoadCache').mockReturnValue(cachedVersion);
const result = await getLatestFuelsVersion();
expect(result).toEqual('1.0.0');
});

it('should fetch if there is no cache or the cache is expired', async () => {
const mockResponse = new Response(JSON.stringify({ version: '1.0.0' }));
const fetchSpy = vi.spyOn(global, 'fetch').mockReturnValue(Promise.resolve(mockResponse));
const saveCacheSpy = vi.spyOn(cacheMod, 'saveToCache').mockImplementation(() => {});
vi.spyOn(cacheMod, 'checkAndLoadCache').mockReturnValue(null);
const version = await getLatestFuelsVersion();
expect(fetchSpy).toHaveBeenCalled();
expect(version).toEqual('1.0.0');
expect(saveCacheSpy).toHaveBeenCalledWith('1.0.0');
Copy link
Member

Choose a reason for hiding this comment

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

Because [I'm presuming] you're mocking everything related to the cache, the tests may not cover some critical parts of the functionality. Can we get all files in 100%?

coverage

Copy link
Contributor

Choose a reason for hiding this comment

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

Think this relates - we should cover both success and failure conditions in my opinion.

});
});
25 changes: 25 additions & 0 deletions packages/fuels/src/cli/utils/getLatestFuelsVersion.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { checkAndLoadCache, saveToCache } from './fuelsVersionCache';

export const getLatestFuelsVersion = async (): Promise<string | undefined> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we potentially document the throwable conditions here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if I follow you, can you share some pseudo code? What do you mean by documenting the throwable conditions?

const cachedVersion = checkAndLoadCache();
if (cachedVersion) {
return cachedVersion;
}

const data: { version: string } | null = await Promise.race([
new Promise((resolve) => {
setTimeout(() => resolve(null), 3000);
}),
arboleya marked this conversation as resolved.
Show resolved Hide resolved
fetch('https://registry.npmjs.org/fuels/latest').then((response) => response.json()),
]);

if (!data) {
throw new Error('Failed to fetch latest fuels version.');
}
arboleya marked this conversation as resolved.
Show resolved Hide resolved

const version = data.version as string;

saveToCache(version);

return version;
};
3 changes: 2 additions & 1 deletion packages/fuels/src/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { checkForAndDisplayUpdates } from './cli/utils/checkForAndDisplayUpdates
import { error } from './cli/utils/logger';

export const run = async (argv: string[]) => {
await checkForAndDisplayUpdates().catch(error);
const program = configureCli();
return Promise.all([await checkForAndDisplayUpdates().catch(error), program.parseAsync(argv)]);
return program.parseAsync(argv);
};
5 changes: 5 additions & 0 deletions packages/fuels/test/features/build.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { join } from 'path';

import * as deployMod from '../../src/cli/commands/deploy/index';
import { mockStartFuelCore } from '../utils/mockAutoStartFuelCore';
import { mockCheckForUpdates } from '../utils/mockCheckForUpdates';
import {
bootstrapProject,
resetConfigAndMocks,
Expand All @@ -17,6 +18,10 @@ import {
describe('build', { timeout: 180000 }, () => {
const paths = bootstrapProject(__filename);

beforeEach(() => {
mockCheckForUpdates();
});

afterEach(() => {
resetConfigAndMocks(paths.fuelsConfigPath);
});
Expand Down
2 changes: 2 additions & 0 deletions packages/fuels/test/features/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { existsSync, readFileSync, writeFileSync } from 'fs';
import { join } from 'path';

import { launchTestNode } from '../../src/test-utils';
import { mockCheckForUpdates } from '../utils/mockCheckForUpdates';
import { resetDiskAndMocks } from '../utils/resetDiskAndMocks';
import {
bootstrapProject,
Expand All @@ -24,6 +25,7 @@ describe('deploy', { timeout: 180000 }, () => {
resetConfigAndMocks(paths.fuelsConfigPath);
resetDiskAndMocks(paths.root);
paths = bootstrapProject(__filename);
mockCheckForUpdates();
});

afterEach(() => {
Expand Down
5 changes: 5 additions & 0 deletions packages/fuels/test/features/dev.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as chokidar from 'chokidar';
import * as buildMod from '../../src/cli/commands/build/index';
import * as deployMod from '../../src/cli/commands/deploy/index';
import { mockStartFuelCore } from '../utils/mockAutoStartFuelCore';
import { mockCheckForUpdates } from '../utils/mockCheckForUpdates';
import { mockLogger } from '../utils/mockLogger';
import { resetDiskAndMocks } from '../utils/resetDiskAndMocks';
import { runInit, runDev, bootstrapProject, resetConfigAndMocks } from '../utils/runCommands';
Expand All @@ -21,6 +22,10 @@ vi.mock('chokidar', async () => {
describe('dev', () => {
const paths = bootstrapProject(__filename);

beforeEach(() => {
mockCheckForUpdates();
});

afterEach(() => {
resetConfigAndMocks(paths.fuelsConfigPath);
});
Expand Down
2 changes: 2 additions & 0 deletions packages/fuels/test/features/init.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import chalk from 'chalk';
import { existsSync, readFileSync } from 'fs';

import { Commands } from '../../src';
import { mockCheckForUpdates } from '../utils/mockCheckForUpdates';
import { mockLogger } from '../utils/mockLogger';
import {
bootstrapProject,
Expand All @@ -19,6 +20,7 @@ describe('init', () => {

beforeEach(() => {
mockLogger();
mockCheckForUpdates();
});

afterEach(() => {
Expand Down
7 changes: 7 additions & 0 deletions packages/fuels/test/utils/mockCheckForUpdates.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import * as checkForUpdatesMod from '../../src/cli/utils/checkForAndDisplayUpdates';

export const mockCheckForUpdates = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we always mock a successful resolution?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because as of now this helper for the mock is only used in places where we always expect this functionality to succeed or not run.

vi.spyOn(checkForUpdatesMod, 'checkForAndDisplayUpdates').mockImplementation(() =>
Promise.resolve()
);
};
Loading