From 03fb93e3c9f233954fd4dddf8aa13b33c64ba8a0 Mon Sep 17 00:00:00 2001 From: Paulo Margarido <64600052+paulomarg@users.noreply.github.com> Date: Mon, 9 Dec 2024 11:22:18 -0500 Subject: [PATCH 1/4] Remove unused lockfiles after clone --- .changeset/silver-mice-thank.md | 6 +++ packages/app/src/cli/services/init/init.ts | 2 +- .../services/init/template/cleanup.test.ts | 43 ++++++++++++++++++- .../src/cli/services/init/template/cleanup.ts | 24 +++++++++-- packages/cli-kit/src/public/node/fs.ts | 15 ++++++- .../src/public/node/node-package-manager.ts | 7 +++ 6 files changed, 89 insertions(+), 8 deletions(-) create mode 100644 .changeset/silver-mice-thank.md diff --git a/.changeset/silver-mice-thank.md b/.changeset/silver-mice-thank.md new file mode 100644 index 00000000000..3cb763c622f --- /dev/null +++ b/.changeset/silver-mice-thank.md @@ -0,0 +1,6 @@ +--- +'@shopify/cli-kit': patch +'@shopify/app': patch +--- + +Remove all template lockfiles, except the one used to install dependencies diff --git a/packages/app/src/cli/services/init/init.ts b/packages/app/src/cli/services/init/init.ts index 6cdfa3417f9..1a3e2659585 100644 --- a/packages/app/src/cli/services/init/init.ts +++ b/packages/app/src/cli/services/init/init.ts @@ -180,7 +180,7 @@ async function init(options: InitOptions) { { title: 'Cleaning up', task: async () => { - await cleanup(templateScaffoldDir) + await cleanup(templateScaffoldDir, packageManager) }, }, { diff --git a/packages/app/src/cli/services/init/template/cleanup.test.ts b/packages/app/src/cli/services/init/template/cleanup.test.ts index 51a4240c06c..a63e93ca24d 100644 --- a/packages/app/src/cli/services/init/template/cleanup.test.ts +++ b/packages/app/src/cli/services/init/template/cleanup.test.ts @@ -2,6 +2,7 @@ import cleanup from './cleanup.js' import {describe, expect, test} from 'vitest' import {inTemporaryDirectory, mkdir, writeFile, fileExists} from '@shopify/cli-kit/node/fs' import {joinPath} from '@shopify/cli-kit/node/path' +import {PackageManager} from '@shopify/cli-kit/node/node-package-manager' describe('cleanup', () => { async function mockProjectFolder(tmpDir: string) { @@ -35,7 +36,7 @@ describe('cleanup', () => { await mockProjectFolder(tmpDir) // When - await cleanup(tmpDir) + await cleanup(tmpDir, 'npm') // Then await expect(fileExists(joinPath(tmpDir, '.git'))).resolves.toBe(false) @@ -52,7 +53,7 @@ describe('cleanup', () => { await mockProjectFolder(tmpDir) // When - await cleanup(tmpDir) + await cleanup(tmpDir, 'npm') // Then await expect(fileExists(joinPath(tmpDir, 'server.js'))).resolves.toBe(true) @@ -60,4 +61,42 @@ describe('cleanup', () => { await expect(fileExists(joinPath(tmpDir, 'frontend', 'node_modules'))).resolves.toBe(true) }) }) + + test.each(['npm', 'yarn', 'pnpm', 'bun'])('deletes unused lockfiles for %s', async (packageManager) => { + await inTemporaryDirectory(async (tmpDir) => { + await mockProjectFolder(tmpDir) + await writeFile(joinPath(tmpDir, 'package-lock.json'), '{}') + await writeFile(joinPath(tmpDir, 'yarn.lock'), '{}') + await writeFile(joinPath(tmpDir, 'pnpm-lock.yaml'), '{}') + await writeFile(joinPath(tmpDir, 'bun.lockb'), '{}') + + // When + await cleanup(tmpDir, packageManager as PackageManager) + + // Then + await expect(fileExists(joinPath(tmpDir, 'package-lock.json'))).resolves.toBe(packageManager === 'npm') + await expect(fileExists(joinPath(tmpDir, 'yarn.lock'))).resolves.toBe(packageManager === 'yarn') + await expect(fileExists(joinPath(tmpDir, 'pnpm-lock.yaml'))).resolves.toBe(packageManager === 'pnpm') + await expect(fileExists(joinPath(tmpDir, 'bun.lockb'))).resolves.toBe(packageManager === 'bun') + }) + }) + + test('does not delete lockfiles for unknown package manager', async () => { + await inTemporaryDirectory(async (tmpDir) => { + await mockProjectFolder(tmpDir) + await writeFile(joinPath(tmpDir, 'package-lock.json'), '{}') + await writeFile(joinPath(tmpDir, 'yarn.lock'), '{}') + await writeFile(joinPath(tmpDir, 'pnpm-lock.yaml'), '{}') + await writeFile(joinPath(tmpDir, 'bun.lockb'), '{}') + + // When + await cleanup(tmpDir, 'unknown') + + // Then + await expect(fileExists(joinPath(tmpDir, 'package-lock.json'))).resolves.toBe(true) + await expect(fileExists(joinPath(tmpDir, 'yarn.lock'))).resolves.toBe(true) + await expect(fileExists(joinPath(tmpDir, 'pnpm-lock.yaml'))).resolves.toBe(true) + await expect(fileExists(joinPath(tmpDir, 'bun.lockb'))).resolves.toBe(true) + }) + }) }) diff --git a/packages/app/src/cli/services/init/template/cleanup.ts b/packages/app/src/cli/services/init/template/cleanup.ts index fa46e90b370..bb1b9cedaea 100644 --- a/packages/app/src/cli/services/init/template/cleanup.ts +++ b/packages/app/src/cli/services/init/template/cleanup.ts @@ -1,7 +1,8 @@ -import {rmdir, glob} from '@shopify/cli-kit/node/fs' +import {rmdir, glob, fileExistsSync, unlinkFile} from '@shopify/cli-kit/node/fs' +import {lockfilesByManager, PackageManager} from '@shopify/cli-kit/node/node-package-manager' import {joinPath} from '@shopify/cli-kit/node/path' -export default async function cleanup(webOutputDirectory: string) { +export default async function cleanup(webOutputDirectory: string, packageManager: PackageManager) { const gitPaths = await glob( [ joinPath(webOutputDirectory, '**', '.git'), @@ -20,5 +21,22 @@ export default async function cleanup(webOutputDirectory: string) { }, ) - return Promise.all(gitPaths.map((path) => rmdir(path, {force: true}))).then(() => {}) + const unusedLockfiles = + packageManager === 'unknown' + ? [] + : Object.entries(lockfilesByManager).reduce((acc, [manager, lockfile]) => { + if (manager !== 'unknown' && manager !== packageManager) { + const path = joinPath(webOutputDirectory, lockfile) + if (fileExistsSync(path)) { + acc.push(path) + } + } + + return acc + }, []) + + return Promise.all([ + ...gitPaths.map((path) => rmdir(path, {force: true})), + ...unusedLockfiles.map((path) => unlinkFile(path)), + ]).then(() => {}) } diff --git a/packages/cli-kit/src/public/node/fs.ts b/packages/cli-kit/src/public/node/fs.ts index 44799515c9f..b9ee1291900 100644 --- a/packages/cli-kit/src/public/node/fs.ts +++ b/packages/cli-kit/src/public/node/fs.ts @@ -43,6 +43,7 @@ import { chmod as fsChmod, access as fsAccess, rename as fsRename, + unlink as fsUnlink, } from 'fs/promises' import {pathToFileURL as pathToFile} from 'url' import * as os from 'os' @@ -329,14 +330,24 @@ export function fileSizeSync(path: string): number { } /** - * Unlink a file at the given path. + * Synchronously unlink a file at the given path. + * * @param path - Path to the file. - * @returns A promise that resolves when the file is unlinked. */ export function unlinkFileSync(path: string): void { fsUnlinkSync(path) } +/** + * Unlink a file at the given path. + * + * @param path - Path to the file. + * @returns A promise that resolves when the file is unlinked. + */ +export function unlinkFile(path: string): Promise { + return fsUnlink(path) +} + /** * Create a read stream for a file with optional options. * diff --git a/packages/cli-kit/src/public/node/node-package-manager.ts b/packages/cli-kit/src/public/node/node-package-manager.ts index 99094948dd1..b25bc6eb0f5 100644 --- a/packages/cli-kit/src/public/node/node-package-manager.ts +++ b/packages/cli-kit/src/public/node/node-package-manager.ts @@ -29,6 +29,13 @@ export const pnpmWorkspaceFile = 'pnpm-workspace.yaml' /** An array containing the lockfiles from all the package managers */ export const lockfiles: Lockfile[] = [yarnLockfile, pnpmLockfile, npmLockfile, bunLockfile] +export const lockfilesByManager: {[key in PackageManager]: Lockfile} = { + yarn: yarnLockfile, + npm: npmLockfile, + pnpm: pnpmLockfile, + bun: bunLockfile, + unknown: yarnLockfile, +} export type Lockfile = 'yarn.lock' | 'package-lock.json' | 'pnpm-lock.yaml' | 'bun.lockb' /** From 7b56b4b5fbe2726b4e22dcb8c868dc837b21abd3 Mon Sep 17 00:00:00 2001 From: Paulo Margarido <64600052+paulomarg@users.noreply.github.com> Date: Mon, 9 Dec 2024 13:06:10 -0500 Subject: [PATCH 2/4] Applying comment from review --- packages/app/src/cli/services/init/template/cleanup.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/app/src/cli/services/init/template/cleanup.ts b/packages/app/src/cli/services/init/template/cleanup.ts index bb1b9cedaea..29f8e595811 100644 --- a/packages/app/src/cli/services/init/template/cleanup.ts +++ b/packages/app/src/cli/services/init/template/cleanup.ts @@ -38,5 +38,5 @@ export default async function cleanup(webOutputDirectory: string, packageManager return Promise.all([ ...gitPaths.map((path) => rmdir(path, {force: true})), ...unusedLockfiles.map((path) => unlinkFile(path)), - ]).then(() => {}) + ]) } From 3b835f06c050d1d61080cc6702f6f7fc73ae4fe0 Mon Sep 17 00:00:00 2001 From: Paulo Margarido <64600052+paulomarg@users.noreply.github.com> Date: Tue, 10 Dec 2024 09:05:33 -0500 Subject: [PATCH 3/4] Applying comments from code review --- .../src/cli/services/init/template/cleanup.ts | 27 ++++++++----------- .../src/public/node/node-package-manager.ts | 4 +-- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/packages/app/src/cli/services/init/template/cleanup.ts b/packages/app/src/cli/services/init/template/cleanup.ts index 29f8e595811..5596d7d5bcc 100644 --- a/packages/app/src/cli/services/init/template/cleanup.ts +++ b/packages/app/src/cli/services/init/template/cleanup.ts @@ -1,5 +1,5 @@ import {rmdir, glob, fileExistsSync, unlinkFile} from '@shopify/cli-kit/node/fs' -import {lockfilesByManager, PackageManager} from '@shopify/cli-kit/node/node-package-manager' +import {Lockfile, lockfilesByManager, PackageManager} from '@shopify/cli-kit/node/node-package-manager' import {joinPath} from '@shopify/cli-kit/node/path' export default async function cleanup(webOutputDirectory: string, packageManager: PackageManager) { @@ -21,22 +21,17 @@ export default async function cleanup(webOutputDirectory: string, packageManager }, ) - const unusedLockfiles = + const gitPathPromises = gitPaths.map((path) => rmdir(path, {force: true})) + + const lockfilePromises = packageManager === 'unknown' ? [] - : Object.entries(lockfilesByManager).reduce((acc, [manager, lockfile]) => { - if (manager !== 'unknown' && manager !== packageManager) { - const path = joinPath(webOutputDirectory, lockfile) - if (fileExistsSync(path)) { - acc.push(path) - } - } - - return acc - }, []) + : Object.entries(lockfilesByManager) + .filter(([manager, lockfile]) => manager !== packageManager && lockfile) + .map(([_, lockfile]) => { + const path = joinPath(webOutputDirectory, lockfile as Lockfile) + if (fileExistsSync(path)) return unlinkFile(path) + }, []) - return Promise.all([ - ...gitPaths.map((path) => rmdir(path, {force: true})), - ...unusedLockfiles.map((path) => unlinkFile(path)), - ]) + return Promise.all([...gitPathPromises, ...lockfilePromises]) } diff --git a/packages/cli-kit/src/public/node/node-package-manager.ts b/packages/cli-kit/src/public/node/node-package-manager.ts index b25bc6eb0f5..89dcb0ca417 100644 --- a/packages/cli-kit/src/public/node/node-package-manager.ts +++ b/packages/cli-kit/src/public/node/node-package-manager.ts @@ -29,12 +29,12 @@ export const pnpmWorkspaceFile = 'pnpm-workspace.yaml' /** An array containing the lockfiles from all the package managers */ export const lockfiles: Lockfile[] = [yarnLockfile, pnpmLockfile, npmLockfile, bunLockfile] -export const lockfilesByManager: {[key in PackageManager]: Lockfile} = { +export const lockfilesByManager: {[key in PackageManager]: Lockfile | undefined} = { yarn: yarnLockfile, npm: npmLockfile, pnpm: pnpmLockfile, bun: bunLockfile, - unknown: yarnLockfile, + unknown: undefined, } export type Lockfile = 'yarn.lock' | 'package-lock.json' | 'pnpm-lock.yaml' | 'bun.lockb' From 85bc807789594b2e9637a6c1e10cb0ab90e90527 Mon Sep 17 00:00:00 2001 From: Paulo Margarido <64600052+paulomarg@users.noreply.github.com> Date: Tue, 10 Dec 2024 11:08:45 -0500 Subject: [PATCH 4/4] Remove all lock files when manager is unknown --- .../cli/services/init/template/cleanup.test.ts | 10 +++++----- .../app/src/cli/services/init/template/cleanup.ts | 15 ++++++--------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/packages/app/src/cli/services/init/template/cleanup.test.ts b/packages/app/src/cli/services/init/template/cleanup.test.ts index a63e93ca24d..37fe613d02e 100644 --- a/packages/app/src/cli/services/init/template/cleanup.test.ts +++ b/packages/app/src/cli/services/init/template/cleanup.test.ts @@ -81,7 +81,7 @@ describe('cleanup', () => { }) }) - test('does not delete lockfiles for unknown package manager', async () => { + test('deletes all lockfiles for unknown package manager', async () => { await inTemporaryDirectory(async (tmpDir) => { await mockProjectFolder(tmpDir) await writeFile(joinPath(tmpDir, 'package-lock.json'), '{}') @@ -93,10 +93,10 @@ describe('cleanup', () => { await cleanup(tmpDir, 'unknown') // Then - await expect(fileExists(joinPath(tmpDir, 'package-lock.json'))).resolves.toBe(true) - await expect(fileExists(joinPath(tmpDir, 'yarn.lock'))).resolves.toBe(true) - await expect(fileExists(joinPath(tmpDir, 'pnpm-lock.yaml'))).resolves.toBe(true) - await expect(fileExists(joinPath(tmpDir, 'bun.lockb'))).resolves.toBe(true) + await expect(fileExists(joinPath(tmpDir, 'package-lock.json'))).resolves.toBe(false) + await expect(fileExists(joinPath(tmpDir, 'yarn.lock'))).resolves.toBe(false) + await expect(fileExists(joinPath(tmpDir, 'pnpm-lock.yaml'))).resolves.toBe(false) + await expect(fileExists(joinPath(tmpDir, 'bun.lockb'))).resolves.toBe(false) }) }) }) diff --git a/packages/app/src/cli/services/init/template/cleanup.ts b/packages/app/src/cli/services/init/template/cleanup.ts index 5596d7d5bcc..fde535065e4 100644 --- a/packages/app/src/cli/services/init/template/cleanup.ts +++ b/packages/app/src/cli/services/init/template/cleanup.ts @@ -23,15 +23,12 @@ export default async function cleanup(webOutputDirectory: string, packageManager const gitPathPromises = gitPaths.map((path) => rmdir(path, {force: true})) - const lockfilePromises = - packageManager === 'unknown' - ? [] - : Object.entries(lockfilesByManager) - .filter(([manager, lockfile]) => manager !== packageManager && lockfile) - .map(([_, lockfile]) => { - const path = joinPath(webOutputDirectory, lockfile as Lockfile) - if (fileExistsSync(path)) return unlinkFile(path) - }, []) + const lockfilePromises = Object.entries(lockfilesByManager) + .filter(([manager, lockfile]) => manager !== packageManager && lockfile) + .map(([_, lockfile]) => { + const path = joinPath(webOutputDirectory, lockfile as Lockfile) + if (fileExistsSync(path)) return unlinkFile(path) + }, []) return Promise.all([...gitPathPromises, ...lockfilePromises]) }