Skip to content

chore: remove unnecessary abstractions of filesystem functions #5320

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

Merged
merged 3 commits into from
Dec 19, 2022
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
1 change: 0 additions & 1 deletion npm-shrinkwrap.json

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

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,6 @@
"cron-parser": "^4.2.1",
"debug": "^4.1.1",
"decache": "^4.6.0",
"del": "^6.0.0",
"dot-prop": "^6.0.0",
"dotenv": "^16.0.0",
"env-paths": "^2.2.0",
Expand Down
34 changes: 1 addition & 33 deletions src/lib/fs.cjs
Original file line number Diff line number Diff line change
@@ -1,26 +1,8 @@
// @ts-check
const {
constants,
promises: { access, readFile, rm, stat },
promises: { access, stat },
} = require('fs')
const { version } = require('process')

const del = require('del')
const { gte, parse } = require('semver')

const NODE_VERSION = parse(version)

/**
* reads a file async and catches potential errors
* @param {string} filePath
*/
const readFileAsyncCatchError = async (filePath) => {
try {
return { content: await readFile(filePath, 'utf-8') }
} catch (error) {
return { error }
}
}

const fileExistsAsync = async (filePath) => {
try {
Expand All @@ -31,18 +13,6 @@ const fileExistsAsync = async (filePath) => {
}
}

/**
* Removes a directory recursively and async
* @param {string} path
* @returns {Promise<void>}
*/
const rmdirRecursiveAsync = async (path) => {
if (gte(NODE_VERSION, '14.14.0')) {
return await rm(path, { force: true, recursive: true })
}
await del(path, { force: true })
}

/**
* calls stat async with a function and catches potential errors
* @param {string} filePath
Expand Down Expand Up @@ -78,6 +48,4 @@ module.exports = {
fileExistsAsync,
isDirectoryAsync,
isFileAsync,
readFileAsyncCatchError,
rmdirRecursiveAsync,
}
5 changes: 3 additions & 2 deletions src/utils/deploy/deploy-site.mjs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { rm } from 'fs/promises'

import cleanDeep from 'clean-deep'
import tempy from 'tempy'

import { deployFileNormalizer, getDistPathIfExists, isEdgeFunctionFile } from '../../lib/edge-functions/deploy.mjs'
import { rmdirRecursiveAsync } from '../../lib/fs.cjs'
import { warn } from '../command-helpers.mjs'

import {
Expand Down Expand Up @@ -170,7 +171,7 @@ For more information, visit https://ntl.fyi/cli-native-modules.`)
phase: 'stop',
})

await rmdirRecursiveAsync(tmpDir)
await rm(tmpDir, { force: true, recursive: true })

const deployManifest = {
deployId,
Expand Down
9 changes: 4 additions & 5 deletions src/utils/detect-server-settings.mjs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @ts-check
import { readFile } from 'fs/promises'
import { EOL } from 'os'
import path from 'path'
import process from 'process'
Expand All @@ -8,8 +9,6 @@ import fuzzy from 'fuzzy'
import getPort from 'get-port'
import isPlainObject from 'is-plain-obj'

import { readFileAsyncCatchError } from '../lib/fs.cjs'

import { NETLIFYDEVWARN, chalk, log } from './command-helpers.mjs'
import { acquirePort } from './dev.mjs'
import { getInternalFunctionsDir } from './functions/index.mjs'
Expand All @@ -34,9 +33,9 @@ const readHttpsSettings = async (options) => {
throw new TypeError(`Certificate file configuration should be a string`)
}

const [{ content: key, error: keyError }, { content: cert, error: certError }] = await Promise.all([
readFileAsyncCatchError(keyFile),
readFileAsyncCatchError(certFile),
const [{ reason: keyError, value: key }, { reason: certError, value: cert }] = await Promise.allSettled([
readFile(keyFile, 'utf-8'),
readFile(certFile, 'utf-8'),
])

if (keyError) {
Expand Down
8 changes: 4 additions & 4 deletions src/utils/lm/install.mjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// @ts-check
import { appendFile, copyFile, readFile, writeFile } from 'fs/promises'
import { appendFile, copyFile, readFile, rm, writeFile } from 'fs/promises'
import os from 'os'
import path, { dirname } from 'path'
import process from 'process'
Expand All @@ -11,7 +11,7 @@ import Listr from 'listr'
import pathKey from 'path-key'

import { fetchLatestVersion, shouldFetchLatestVersion } from '../../lib/exec-fetcher.mjs'
import { fileExistsAsync, rmdirRecursiveAsync } from '../../lib/fs.cjs'
import { fileExistsAsync } from '../../lib/fs.cjs'
import { normalizeBackslash } from '../../lib/path.mjs'
import { getLegacyPathInHome, getPathInHome } from '../../lib/settings.cjs'
import { chalk } from '../command-helpers.mjs'
Expand Down Expand Up @@ -92,7 +92,7 @@ const installedWithPackageManager = async function () {

const installHelper = async function () {
// remove any old versions that might still exist in `~/.netlify/helper/bin`
await rmdirRecursiveAsync(getLegacyBinPath())
await rm(getLegacyBinPath(), { force: true, recursive: true })
const binPath = getBinPath()
const shouldFetch = await shouldFetchLatestVersion({
binPath,
Expand Down Expand Up @@ -277,7 +277,7 @@ const cleanupShell = async function () {

export const uninstall = async function () {
await Promise.all([
rmdirRecursiveAsync(getHelperPath()),
rm(getHelperPath(), { force: true, recursive: true }),
removeConfig(GIT_CONFIG, getGitConfigContent(getGitConfigPath())),
cleanupShell(),
])
Expand Down
13 changes: 4 additions & 9 deletions tests/integration/640.command.recipes.test.cjs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
const { readFile } = require('fs/promises')
const { resolve } = require('path')

const test = require('ava')
const execa = require('execa')

const { readFileAsyncCatchError } = require('../../src/lib/fs.cjs')

const callCli = require('./utils/call-cli.cjs')
const cliPath = require('./utils/cli-path.cjs')
const { CONFIRM, answerWithValue, handleQuestions } = require('./utils/handle-questions.cjs')
Expand Down Expand Up @@ -35,10 +34,8 @@ test('Generates a new VS Code settings file if one does not exist', async (t) =>

await childProcess

const { content, error } = await readFileAsyncCatchError(`${builder.directory}/.vscode/settings.json`)
const settings = JSON.parse(content)
const settings = JSON.parse(await readFile(`${builder.directory}/.vscode/settings.json`))

t.is(error, undefined)
t.is(settings['deno.enable'], true)
t.is(settings['deno.importMap'], '.netlify/edge-functions-import-map.json')
t.deepEqual(settings['deno.enablePaths'], ['netlify/edge-functions'])
Expand Down Expand Up @@ -68,10 +65,8 @@ test('Updates an existing VS Code settings file', async (t) => {

await childProcess

const { content, error } = await readFileAsyncCatchError(`${builder.directory}/.vscode/settings.json`)
const settings = JSON.parse(content)
const settings = JSON.parse(await readFile(`${builder.directory}/.vscode/settings.json`))

t.is(error, undefined)
t.is(settings.someSetting, 'value')
t.is(settings['deno.enable'], true)
t.is(settings['deno.importMap'], '.netlify/edge-functions-import-map.json')
Expand All @@ -97,7 +92,7 @@ test('Does not generate a new VS Code settings file if the user does not confirm

await childProcess

const { error } = await readFileAsyncCatchError(`${builder.directory}/.vscode/settings.json`)
const error = await t.throwsAsync(() => readFile(`${builder.directory}/.vscode/settings.json`))
t.is(error.code, 'ENOENT')
})
})
6 changes: 2 additions & 4 deletions tests/integration/utils/mock-execa.cjs
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
const { writeFile } = require('fs').promises
const { rm, writeFile } = require('fs').promises
const { pathToFileURL } = require('url')

const tempy = require('tempy')

const { rmdirRecursiveAsync } = require('../../../src/lib/fs.cjs')

// Saves to disk a JavaScript file with the contents provided and returns
// an environment variable that replaces the `execa` module implementation.
// A cleanup method is also returned, allowing the consumer to remove the
Expand All @@ -20,7 +18,7 @@ const createMock = async (contents) => {
}
const cleanup = () =>
// eslint-disable-next-line promise/prefer-await-to-then
rmdirRecursiveAsync(path).catch(() => {
rm(path, { force: true, recursive: true }).catch(() => {
// no-op
})

Expand Down
6 changes: 2 additions & 4 deletions tests/integration/utils/site-builder.cjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// @ts-check
const { copyFile, mkdir, unlink, writeFile } = require('fs').promises
const { copyFile, mkdir, rm, unlink, writeFile } = require('fs').promises
const os = require('os')
const path = require('path')
const process = require('process')
Expand All @@ -10,8 +10,6 @@ const tempDirectory = require('temp-dir')
const { toToml } = require('tomlify-j0.4')
const { v4: uuidv4 } = require('uuid')

const { rmdirRecursiveAsync } = require('../../../src/lib/fs.cjs')

const ensureDir = (file) => mkdir(file, { recursive: true })

const createSiteBuilder = ({ siteName }) => {
Expand Down Expand Up @@ -184,7 +182,7 @@ const createSiteBuilder = ({ siteName }) => {
return builder
},
cleanupAsync: async () => {
await rmdirRecursiveAsync(directory).catch((error) => {
await rm(directory, { force: true, recursive: true }).catch((error) => {
console.warn(error)
})
return builder
Expand Down
13 changes: 6 additions & 7 deletions tests/unit/utils/get-global-config.test.mjs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { copyFile, mkdir, readFile, unlink, writeFile } from 'fs/promises'
import { copyFile, mkdir, readFile, rm, unlink, writeFile } from 'fs/promises'
import os from 'os'
import { join } from 'path'

import { afterAll, beforeAll, beforeEach, expect, test } from 'vitest'

import { rmdirRecursiveAsync } from '../../../src/lib/fs.cjs'
import { getLegacyPathInHome, getPathInHome } from '../../../src/lib/settings.cjs'
import getGlobalConfig, { resetConfigCache } from '../../../src/utils/get-global-config.mjs'

Expand All @@ -28,13 +27,13 @@ afterAll(async () => {
await unlink(tmpConfigBackupPath)
} catch {}
// Remove legacy config path
await rmdirRecursiveAsync(getLegacyPathInHome([]))
await rm(getLegacyPathInHome([]), { force: true, recursive: true })
})

beforeEach(async () => {
// Remove config dirs
await rmdirRecursiveAsync(getPathInHome([]))
await rmdirRecursiveAsync(getLegacyPathInHome([]))
await rm(getPathInHome([]), { force: true, recursive: true })
await rm(getLegacyPathInHome([]), { force: true, recursive: true })
// Make config dirs
await mkdir(getPathInHome([]))
await mkdir(getLegacyPathInHome([]))
Expand Down Expand Up @@ -63,8 +62,8 @@ test('should not throw if legacy config is invalid JSON', async () => {

test("should create config in netlify's config dir if none exists and store new values", async () => {
// Remove config dirs
await rmdirRecursiveAsync(getPathInHome([]))
await rmdirRecursiveAsync(getLegacyPathInHome([]))
await rm(getPathInHome([]), { force: true, recursive: true })
await rm(getLegacyPathInHome([]), { force: true, recursive: true })
const globalConfig = await getGlobalConfig()
globalConfig.set('newProp', 'newValue')
const configFile = JSON.parse(await readFile(configPath, 'utf-8'))
Expand Down
28 changes: 11 additions & 17 deletions tools/e2e/setup.mjs
Original file line number Diff line number Diff line change
@@ -1,22 +1,15 @@
import { appendFileSync, existsSync, promises, readFileSync, writeFileSync } from 'fs'
import { appendFile, mkdtemp, readFile, rm, writeFile } from 'fs/promises'
Copy link
Contributor Author

@danez danez Dec 16, 2022

Choose a reason for hiding this comment

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

I updated this file to use async version of the fs functions.

import { tmpdir } from 'os'
import { join, sep } from 'path'
import { cwd, env } from 'process'
import { fileURLToPath } from 'url'

import del from 'del'
import execa from 'execa'
import getPort from 'get-port'
import verdaccio from 'verdaccio'

// TODO: remove this once `../../src/lib/fs.js` is an esm module as well
Copy link
Contributor Author

@danez danez Dec 16, 2022

Choose a reason for hiding this comment

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

Don't know why we should wait, ESM can import commonjs

const rmdirRecursiveAsync = async (path) => {
await del(path, { force: true })
}

const { mkdtemp } = promises
import { fileExistsAsync } from '../../src/lib/fs.cjs'

// eslint-disable-next-line no-magic-numbers
const VERDACCIO_TIMEOUT_MILLISECONDS = 60 * 1000
const START_PORT_RANGE = 5000
const END_PORT_RANGE = 5000
Expand Down Expand Up @@ -70,7 +63,7 @@ export const startRegistry = async () => {
const storage = fileURLToPath(new URL('../../.verdaccio-storage', import.meta.url))

// Remove netlify-cli from the verdaccio storage because we are going to publish it in a second
await rmdirRecursiveAsync(join(storage, 'netlify-cli'))
await rm(join(storage, 'netlify-cli'), { force: true, recursive: true })

return new Promise((resolve, reject) => {
setTimeout(() => {
Expand Down Expand Up @@ -105,17 +98,17 @@ export const setup = async () => {
/** Cleans up everything */
const cleanup = async () => {
// remote temp folders
await rmdirRecursiveAsync(workspace)
await rm(workspace, { force: true, recursive: true })
}

env.npm_config_registry = url

try {
if (existsSync(npmrc)) {
backupNpmrc = readFileSync(npmrc, 'utf-8')
appendFileSync(npmrc, registryWithAuth)
if (await fileExistsAsync(npmrc)) {
backupNpmrc = await readFile(npmrc, 'utf-8')
await appendFile(npmrc, registryWithAuth)
} else {
writeFileSync(npmrc, registryWithAuth, 'utf-8')
await writeFile(npmrc, registryWithAuth, 'utf-8')
}

// publish the CLI package to our registry
Expand All @@ -137,10 +130,11 @@ ${error_ instanceof Error ? error_.message : error_}`,
)
} finally {
// restore .npmrc
// eslint-disable-next-line unicorn/prefer-ternary
if (backupNpmrc) {
writeFileSync(npmrc, backupNpmrc)
await writeFile(npmrc, backupNpmrc)
} else {
await rmdirRecursiveAsync(npmrc)
await rm(npmrc, { force: true, recursive: true })
}
}

Expand Down