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

fix: file write errors for tools and newsroom video #3297

Merged
merged 7 commits into from
Oct 26, 2024
Merged
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
124 changes: 36 additions & 88 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
"clsx": "^2.1.0",
"cssnano": "^6.0.3",
"dotenv": "^16.4.4",
"fs-extra": "^11.2.0",
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Native fs module is still widely used - migration to fs-extra is incomplete

The codebase shows inconsistent usage of file system modules:

  • fs-extra is only used in 4 files: build-tools.js, build-tools.test.js, build-newsroom-videos.js, and build-newsroom-videos.test.js
  • 19 other files still use the native fs module

Since the PR's objective was to switch from native fs to fs-extra for better error handling and async operations, the migration appears to be incomplete. The changes should be consistent across the codebase to:

  • Leverage fs-extra's enhanced features uniformly
  • Maintain consistent error handling patterns
  • Avoid mixing different file operation paradigms
🔗 Analysis chain

LGTM! Consider adding @types/fs-extra for TypeScript support.

The addition of fs-extra is appropriate for enhanced file system operations and better error handling. Since this project uses TypeScript, consider adding @types/fs-extra to devDependencies for improved type safety.

Let's verify the fs-extra usage across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify fs-extra is properly imported and used instead of native fs

# Test 1: Check if native 'fs' is still being imported
echo "Checking for native fs imports..."
rg "require\(['\"]fs['\"]\)" || rg "from ['\"]fs['\"]\""

# Test 2: Verify fs-extra usage
echo "Checking fs-extra usage..."
rg "require\(['\"]fs-extra['\"]\)" || rg "from ['\"]fs-extra['\"]\""

Length of output: 1781

"fuse.js": "^7.0.0",
"googleapis": "^133.0.0",
"gray-matter": "^4.0.3",
Expand Down
4 changes: 2 additions & 2 deletions scripts/build-newsroom-videos.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const { writeFileSync } = require('fs');
const { writeFileSync } = require('fs-extra');
const { resolve } = require('path');
const fetch = require('node-fetch-2');

Expand All @@ -19,7 +19,7 @@ async function buildNewsroomVideos(writePath) {
}

const data = await response.json();
console.log(data)
console.log(data);

if (!data.items || !Array.isArray(data.items)) {
throw new Error('Invalid data structure received from YouTube API');
Expand Down
7 changes: 2 additions & 5 deletions scripts/build-tools.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,15 @@
const { getData } = require('./tools/extract-tools-github');
const { convertTools } = require('./tools/tools-object');
const { combineTools } = require('./tools/combine-tools');
const fs = require('fs');
const fs = require('fs-extra');
const { resolve } = require('path');

const buildTools = async (automatedToolsPath, manualToolsPath, toolsPath, tagsPath) => {
try {
let githubExtractData = await getData();
let automatedTools = await convertTools(githubExtractData);

fs.writeFileSync(
automatedToolsPath,
JSON.stringify(automatedTools, null, ' ')
);
await fs.writeFile(automatedToolsPath, JSON.stringify(automatedTools, null, ' '));

await combineTools(automatedTools, require(manualToolsPath), toolsPath, tagsPath);
} catch (err) {
Expand Down
14 changes: 7 additions & 7 deletions tests/build-newsroom-videos.test.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
const { readFileSync, rmSync, mkdirSync } = require('fs');
const { resolve } = require('path');
const { readFileSync, removeSync, mkdirpSync } = require('fs-extra');
const { resolve, join } = require('path');
const { buildNewsroomVideos } = require('../scripts/build-newsroom-videos');
const { mockApiResponse, expectedResult } = require('./fixtures/newsroomData');
const fetch = require('node-fetch-2');
const os = require('os');

jest.mock('node-fetch-2', () => jest.fn());

describe('buildNewsroomVideos', () => {
const testDir = resolve(__dirname, 'test_config');
const testDir = join(os.tmpdir(), 'test_config');
const testFilePath = resolve(testDir, 'newsroom_videos.json');
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice addition. We should use os.tmpDir() for other scripts as well where a temp folder was being created inside the website directory.


beforeAll(() => {
mkdirSync(testDir, { recursive: true });
mkdirpSync(testDir);
process.env.YOUTUBE_TOKEN = 'testkey';
});

afterAll(() => {
rmSync(testDir, { recursive: true, force: true });
removeSync(testDir);
});

beforeEach(() => {
Expand Down Expand Up @@ -89,13 +90,12 @@ describe('buildNewsroomVideos', () => {
json: jest.fn().mockResolvedValue(mockApiResponse),
});

const invalidPath = '/invalid_dir/newsroom_videos.json';
const invalidPath = resolve(os.tmpdir(), 'invalid_dir', 'newsroom_videos.json');

try {
await buildNewsroomVideos(invalidPath);
} catch (err) {
expect(err.message).toMatch(/ENOENT|EACCES/);
}
});

});
18 changes: 11 additions & 7 deletions tests/build-tools.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ const axios = require('axios');
const { resolve } = require('path');
const { buildTools } = require('../scripts/build-tools');
const { tagsData, manualTools, mockConvertedData, mockExtractData } = require('../tests/fixtures/buildToolsData');
const fs = require('fs');
const fs = require('fs-extra');
const os = require('os');
const path = require('path');

jest.mock('axios');
jest.mock('../scripts/tools/categorylist', () => ({
Expand All @@ -24,19 +26,22 @@ jest.mock('../scripts/tools/tags-color', () => ({
}));

describe('buildTools', () => {
const testDir = resolve(__dirname, 'test_config');
const testDir = path.join(os.tmpdir(), 'test_config');
const toolsPath = resolve(testDir, 'tools.json');
const tagsPath = resolve(testDir, 'all-tags.json');
const automatedToolsPath = resolve(testDir, 'tools-automated.json');
const manualToolsPath = resolve(testDir, 'tools-manual.json');
let consoleErrorMock;

beforeAll(() => {
fs.mkdirSync(testDir, { recursive: true });
fs.writeFileSync(manualToolsPath, JSON.stringify(manualTools));
consoleErrorMock = jest.spyOn(console, 'error').mockImplementation(() => {});
fs.ensureDirSync(testDir);
fs.outputFileSync(manualToolsPath, JSON.stringify(manualTools));
});

afterAll(() => {
fs.rmSync(testDir, { recursive: true, force: true });
fs.removeSync(testDir);
consoleErrorMock.mockRestore();
});

beforeEach(() => {
Expand All @@ -62,7 +67,6 @@ describe('buildTools', () => {
expect(combinedToolsContent["Category2"].description).toEqual(mockConvertedData["Category2"].description);

expect(tagsContent).toEqual(tagsData);

});

it('should handle getData error', async () => {
Expand All @@ -78,7 +82,7 @@ describe('buildTools', () => {
it('should handle file write errors', async () => {
axios.get.mockResolvedValue({ data: mockExtractData });

const invalidPath = '/invalid_dir/tools.json';
const invalidPath = path.resolve(os.tmpdir(), 'invalid_dir', 'tools.json');

try {
await buildTools(invalidPath, manualToolsPath, toolsPath, tagsPath);
Expand Down
Loading