diff --git a/.changeset/graceful-pnpm-blocked-builds-on-generate.md b/.changeset/graceful-pnpm-blocked-builds-on-generate.md new file mode 100644 index 00000000000..1d3ffcc88dc --- /dev/null +++ b/.changeset/graceful-pnpm-blocked-builds-on-generate.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': patch +--- + +Recover gracefully when pnpm blocks dependency build scripts during `app generate extension`: clean up the partially generated extension and explain how to approve the builds diff --git a/.changeset/remove-file-retry-options.md b/.changeset/remove-file-retry-options.md new file mode 100644 index 00000000000..013307a51d1 --- /dev/null +++ b/.changeset/remove-file-retry-options.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': minor +--- + +Add optional `maxRetries` and `retryDelay` options to `removeFile`, passed through to Node's `fs.rm` to retry transient removal errors diff --git a/packages/app/src/cli/services/generate/extension.test.ts b/packages/app/src/cli/services/generate/extension.test.ts index 8c97436e93d..38d25d3f73c 100644 --- a/packages/app/src/cli/services/generate/extension.test.ts +++ b/packages/app/src/cli/services/generate/extension.test.ts @@ -31,6 +31,7 @@ import * as file from '@shopify/cli-kit/node/fs' import * as git from '@shopify/cli-kit/node/git' import {joinPath, dirname} from '@shopify/cli-kit/node/path' import {slugify} from '@shopify/cli-kit/common/string' +import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' vi.mock('../../models/app/validation/multi-cli-warning.js') vi.mock('@shopify/cli-kit/node/node-package-manager', async () => { @@ -459,6 +460,76 @@ describe('initialize a extension', async () => { }) }) + test('cleans up and explains how to recover when pnpm blocks dependency build scripts', async () => { + await withTemporaryApp( + async (tmpDir) => { + // Given + const name = 'my-ext-1' + vi.mocked(installNodeModules).mockRejectedValueOnce( + new Error( + 'Command failed with exit code 1: pnpm install\nERR_PNPM_IGNORED_BUILDS Ignored build scripts: esbuild@0.24.2.', + ), + ) + + // When + const got = createFromTemplate({ + name, + extensionTemplate: checkoutUITemplate, + extensionFlavor: 'vanilla-js', + appDirectory: tmpDir, + specifications, + onGetTemplateRepository, + }) + + // Then + await expect(got).rejects.toThrow( + "Your extension couldn't be generated because pnpm blocked the build scripts of some of its dependencies.", + ) + expect(file.fileExistsSync(joinPath(tmpDir, 'extensions', name))).toBeFalsy() + }, + {useWorkspaces: true}, + ) + }) + + test.skipIf(process.platform === 'win32')( + 'warns and surfaces the original error when the extension directory cleanup fails', + async () => { + await withTemporaryApp( + async (tmpDir) => { + // Given + const name = 'my-ext-1' + const extensionsDirectory = joinPath(tmpDir, 'extensions') + const outputMock = mockAndCaptureOutput() + vi.mocked(installNodeModules).mockImplementationOnce(async () => { + // Make the parent directory read-only so the cleanup can't remove the extension directory. + await file.chmod(extensionsDirectory, 0o555) + throw new Error('pnpm install failed') + }) + + try { + // When + const got = createFromTemplate({ + name, + extensionTemplate: checkoutUITemplate, + extensionFlavor: 'vanilla-js', + appDirectory: tmpDir, + specifications, + onGetTemplateRepository, + }) + + // Then + await expect(got).rejects.toThrow('pnpm install failed') + expect(outputMock.warn()).toContain("Couldn't remove") + } finally { + await file.chmod(extensionsDirectory, 0o755) + outputMock.clear() + } + }, + {useWorkspaces: true}, + ) + }, + ) + test('reloads the app after generating the extension', async () => { await withTemporaryApp(async (tmpDir) => { const downloadGitRepositorySpy = vi.spyOn(git, 'downloadGitRepository').mockResolvedValue() diff --git a/packages/app/src/cli/services/generate/extension.ts b/packages/app/src/cli/services/generate/extension.ts index ab50ff65835..79d3fbbe3cb 100644 --- a/packages/app/src/cli/services/generate/extension.ts +++ b/packages/app/src/cli/services/generate/extension.ts @@ -15,12 +15,14 @@ import { readAndParsePackageJson, } from '@shopify/cli-kit/node/node-package-manager' import {recursiveLiquidTemplateCopy} from '@shopify/cli-kit/node/liquid' -import {renderTasks} from '@shopify/cli-kit/node/ui' +import {renderTasks, renderWarning} from '@shopify/cli-kit/node/ui' import {downloadGitRepository} from '@shopify/cli-kit/node/git' import {fileExists, inTemporaryDirectory, mkdir, moveFile, removeFile, glob} from '@shopify/cli-kit/node/fs' import {joinPath, relativizePath} from '@shopify/cli-kit/node/path' import {slugify} from '@shopify/cli-kit/common/string' import {nonRandomUUID} from '@shopify/cli-kit/node/crypto' +import {AbortError} from '@shopify/cli-kit/node/error' +import {formatPackageManagerCommand} from '@shopify/cli-kit/node/output' export interface GenerateExtensionTemplateOptions { app: AppLinkedInterface @@ -125,11 +127,46 @@ async function extensionInit(options: ExtensionInitOptions) { const lockFilePath = joinPath(options.directory, configurationFileNames.lockFile) await removeFile(lockFilePath) } catch (error) { - await removeFile(options.directory) + await removePartiallyGeneratedExtension(options.directory) + if (options.project.packageManager === 'pnpm' && isPnpmBlockedBuildsError(error)) { + throw new AbortError( + "Your extension couldn't be generated because pnpm blocked the build scripts of some of its dependencies.", + null, + [ + ['Run', {command: 'pnpm approve-builds'}, 'in your app directory to approve the build scripts.'], + [ + 'Run', + {command: formatPackageManagerCommand(options.project.packageManager, 'shopify app generate extension')}, + 'again.', + ], + ], + ) + } throw error } } +// Retries transient removal errors, such as an antivirus lock on the freshly written files. +// If removal still fails, we warn about the leftover directory rather than throwing, so the +// original error isn't lost. +async function removePartiallyGeneratedExtension(directory: string): Promise { + try { + await removeFile(directory, {maxRetries: 10, retryDelay: 100}) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + renderWarning({ + headline: ["Couldn't remove", {filePath: directory}, {char: '.'}], + body: 'Delete this directory manually before generating an extension with the same name.', + }) + } +} + +// pnpm can't prompt to approve build scripts during a non-interactive install like this one, +// so recent versions fail the install outright instead. +function isPnpmBlockedBuildsError(error: unknown): boolean { + return error instanceof Error && error.message.includes('ERR_PNPM_IGNORED_BUILDS') +} + async function themeExtensionInit({ directory, url, diff --git a/packages/cli-kit/src/public/node/fs.ts b/packages/cli-kit/src/public/node/fs.ts index 5948c9f6bb6..cd3aecf4742 100644 --- a/packages/cli-kit/src/public/node/fs.ts +++ b/packages/cli-kit/src/public/node/fs.ts @@ -7,7 +7,6 @@ import { copy as fsCopy, ensureFile as fsEnsureFile, ensureFileSync as fsEnsureFileSync, - remove as fsRemove, removeSync as fsRemoveSync, move as fsMove, // eslint-disable-next-line @typescript-eslint/ban-ts-comment @@ -260,14 +259,29 @@ export function mkdirSync(path: string): void { fsMkdirSync(path, {recursive: true}) } +interface RemoveFileOptions { + /** + * Number of times Node retries the removal when it hits a transient error + * (EBUSY, EMFILE, ENFILE, ENOTEMPTY or EPERM), waiting `retryDelay` milliseconds + * longer on each try. Defaults to 0 (no retries). + */ + maxRetries?: number + /** + * Milliseconds to wait between retries. Defaults to 100. + */ + retryDelay?: number +} + /** - * Removes a file at the given path. + * Removes a file or directory (recursively) at the given path. * - * @param path - Path to the file to be removed. + * @param path - Path to the file or directory to be removed. + * @param options - Retry behavior, passed through to Node's `fs.rm`. Useful when the removal can + * race with transient locks, such as an antivirus scanning freshly written files. */ -export async function removeFile(path: string): Promise { +export async function removeFile(path: string, options: RemoveFileOptions = {}): Promise { outputDebug(outputContent`Removing file at ${outputToken.path(path)}...`) - await fsRemove(path) + await fsRm(path, {recursive: true, force: true, ...options}) } /**