List the uncommitted files that the deploy command errors on (#1944)

* List the uncommitted files that the deploy command errors on * Add separate file to manage package managers * Reorganize deploy tests into describe blocks * Provide next steps for specific package managers
Shopify · Apr 9, 2024 · e3baaba · e3baaba
1 parent 4afedb4
commit e3baaba
Show file tree

Hide file tree

Showing 6 changed files with 201 additions and 104 deletions.
diff --git a/.changeset/twelve-paws-prove.md b/.changeset/twelve-paws-prove.md
@@ -0,0 +1,5 @@
+---
+'@shopify/cli-hydrogen': patch
+---
+
+List the uncommitted files that the deploy command errors on
diff --git a/packages/cli/src/commands/hydrogen/deploy.test.ts b/packages/cli/src/commands/hydrogen/deploy.test.ts
@@ -19,6 +19,7 @@ import {
 
 import {deploymentLogger, runDeploy} from './deploy.js';
 import {getOxygenDeploymentData} from '../../lib/get-oxygen-deployment-data.js';
+import {execAsync} from '../../lib/process.js';
 import {createEnvironmentCliChoiceLabel} from '../../lib/common.js';
 import {
   CompletedDeployment,
@@ -33,20 +34,14 @@ vi.mock('@shopify/cli-kit/node/dot-env');
 vi.mock('@shopify/cli-kit/node/fs');
 vi.mock('@shopify/cli-kit/node/context/local');
 vi.mock('../../lib/get-oxygen-deployment-data.js');
+vi.mock('../../lib/process.js');
 vi.mock('./build-vite.js');
 vi.mock('../../lib/auth.js');
 vi.mock('../../lib/shopify-config.js');
 vi.mock('../../lib/graphql/admin/link-storefront.js');
 vi.mock('../../lib/graphql/admin/create-storefront.js');
 vi.mock('../../lib/graphql/admin/fetch-job.js');
 vi.mock('../../lib/shell.js', () => ({getCliCommand: () => 'h2'}));
-vi.mock('@shopify/cli-kit/node/output', async () => {
-  return {
-    outputContent: () => ({value: ''}),
-    outputInfo: () => {},
-    outputWarn: () => {},
-  };
-});
 vi.mock('@shopify/cli-kit/node/ui', async () => {
   return {
     renderFatalError: vi.fn(),
@@ -320,85 +315,125 @@ describe('deploy', () => {
     ).rejects.toThrowError("Can't specify an environment handle in CI");
   });
 
-  it('errors when there are uncommited changes', async () => {
-    vi.mocked(ensureIsClean).mockRejectedValue(
-      new GitDirectoryNotCleanError('Uncommitted changes'),
-    );
-    await expect(runDeploy(deployParams)).rejects.toThrowError(
-      'Uncommitted changes detected',
-    );
-    expect(vi.mocked(createDeploy)).not.toHaveBeenCalled;
-  });
+  describe('when there are uncommitted changes', () => {
+    it('throws an error', async () => {
+      vi.mocked(execAsync).mockReturnValue(
+        Promise.resolve({stdout: ' M file.ts\n', stderr: ''}) as any,
+      );
 
-  it('proceeds with warning and modified description when there are uncommited changes and the force flag is used', async () => {
-    vi.mocked(ensureIsClean).mockRejectedValue(
-      new GitDirectoryNotCleanError('Uncommitted changes'),
-    );
-    vi.mocked(getLatestGitCommit).mockResolvedValue({
-      hash: '123',
-      message: 'test commit',
-      date: '2021-01-01',
-      author_name: 'test author',
-      author_email: 'test@author.com',
-      body: 'test body',
-      refs: 'HEAD -> main',
+      vi.mocked(ensureIsClean).mockRejectedValue(
+        new GitDirectoryNotCleanError('Uncommitted changes'),
+      );
+      await expect(runDeploy(deployParams)).rejects.toThrowError(
+        'Uncommitted changes detected:\n\n M file.ts',
+      );
+      expect(vi.mocked(createDeploy)).not.toHaveBeenCalled;
     });
 
-    await runDeploy({
-      ...deployParams,
-      force: true,
-    });
+    describe('and there are untracked lockfiles', () => {
+      it('includes additional options for next steps', async () => {
+        vi.mocked(execAsync).mockReturnValue(
+          Promise.resolve({
+            stdout: ' M package-lock.json\n',
+            stderr: '',
+          }) as any,
+        );
 
-    expect(vi.mocked(renderWarning)).toHaveBeenCalledWith({
-      headline: 'No deployment description provided',
-      body: expect.anything(),
-    });
-    expect(vi.mocked(createDeploy)).toHaveBeenCalledWith({
-      config: {
-        ...expectedConfig,
-        environmentTag: 'main',
-        metadata: {
-          ...expectedConfig.metadata,
-          description: '123 with additional changes',
-        },
-      },
-      hooks: expectedHooks,
-      logger: deploymentLogger,
-    });
-  });
+        vi.mocked(ensureIsClean).mockRejectedValue(
+          new GitDirectoryNotCleanError('Uncommitted changes'),
+        );
 
-  it('proceeds with provided description without warning when there are uncommited changes and the force flag is used', async () => {
-    vi.mocked(ensureIsClean).mockRejectedValue(
-      new GitDirectoryNotCleanError('Uncommitted changes'),
-    );
-    vi.mocked(getLatestGitCommit).mockResolvedValue({
-      hash: '123',
-      message: 'test commit',
-      date: '2021-01-01',
-      author_name: 'test author',
-      author_email: 'test@author.com',
-      body: 'test body',
-      refs: 'HEAD -> main',
-    });
+        await expect(runDeploy(deployParams)).rejects.toThrow(
+          expect.objectContaining({
+            message: 'Uncommitted changes detected:\n\n M package-lock.json',
+            nextSteps: expect.arrayContaining([
+              [
+                'If you are using npm, try running',
+                {command: 'npm ci'},
+                'to avoid changes to package-lock.json.',
+              ],
+            ]),
+          }),
+        );
 
-    await runDeploy({
-      ...deployParams,
-      force: true,
-      metadataDescription: 'cool new stuff',
+        expect(vi.mocked(createDeploy)).not.toHaveBeenCalled;
+      });
     });
 
-    expect(vi.mocked(renderWarning)).not.toHaveBeenCalled;
-    expect(vi.mocked(createDeploy)).toHaveBeenCalledWith({
-      config: {
-        ...expectedConfig,
-        environmentTag: 'main',
-        metadata: {
-          ...expectedConfig.metadata,
-          description: 'cool new stuff',
-        },
-      },
-      hooks: expectedHooks,
-      logger: deploymentLogger,
+    describe('and the force flag is used', () => {
+      it('proceeds with a warning and modifies the description', async () => {
+        vi.mocked(ensureIsClean).mockRejectedValue(
+          new GitDirectoryNotCleanError('Uncommitted changes'),
+        );
+        vi.mocked(getLatestGitCommit).mockResolvedValue({
+          hash: '123',
+          message: 'test commit',
+          date: '2021-01-01',
+          author_name: 'test author',
+          author_email: 'test@author.com',
+          body: 'test body',
+          refs: 'HEAD -> main',
+        });
+
+        await runDeploy({
+          ...deployParams,
+          force: true,
+        });
+
+        expect(vi.mocked(renderWarning)).toHaveBeenCalledWith({
+          headline: 'No deployment description provided',
+          body: expect.anything(),
+        });
+        expect(vi.mocked(createDeploy)).toHaveBeenCalledWith({
+          config: {
+            ...expectedConfig,
+            environmentTag: 'main',
+            metadata: {
+              ...expectedConfig.metadata,
+              description: '123 with additional changes',
+            },
+          },
+          hooks: expectedHooks,
+          logger: deploymentLogger,
+        });
+      });
+
+      describe('and a custom description is used', () => {
+        it('uses the provided description', async () => {
+          vi.mocked(ensureIsClean).mockRejectedValue(
+            new GitDirectoryNotCleanError('Uncommitted changes'),
+          );
+          vi.mocked(getLatestGitCommit).mockResolvedValue({
+            hash: '123',
+            message: 'test commit',
+            date: '2021-01-01',
+            author_name: 'test author',
+            author_email: 'test@author.com',
+            body: 'test body',
+            refs: 'HEAD -> main',
+          });
+
+          await runDeploy({
+            ...deployParams,
+            force: true,
+            metadataDescription: 'cool new stuff',
+          });
+
+          expect(vi.mocked(renderWarning)).not.toHaveBeenCalled;
+          expect(vi.mocked(createDeploy)).toHaveBeenCalledWith({
+            config: {
+              ...expectedConfig,
+              environmentTag: 'main',
+              metadata: {
+                ...expectedConfig.metadata,
+                description: 'cool new stuff',
+              },
+            },
+            hooks: expectedHooks,
+            logger: deploymentLogger,
+          });
+        });
+      });
     });
   });
 

diff --git a/packages/cli/src/commands/hydrogen/deploy.ts b/packages/cli/src/commands/hydrogen/deploy.ts
@@ -39,6 +39,7 @@ import {
   findEnvironmentOrThrow,
   orderEnvironmentsBySafety,
 } from '../../lib/common.js';
+import {execAsync} from '../../lib/process.js';
 import {commonFlags, flagsToCamelObject} from '../../lib/flags.js';
 import {getOxygenDeploymentData} from '../../lib/get-oxygen-deployment-data.js';
 import {OxygenDeploymentData} from '../../lib/graphql/admin/get-oxygen-data.js';
@@ -47,6 +48,7 @@ import {runViteBuild} from './build-vite.js';
 import {getViteConfig} from '../../lib/vite-config.js';
 import {prepareDiffDirectory} from '../../lib/template-diff.js';
 import {hasRemixConfigFile} from '../../lib/remix-config.js';
+import {packageManagers} from '../../lib/package-managers.js';
 
 const DEPLOY_OUTPUT_FILE_HANDLE = 'h2_deploy_log.json';
 
@@ -252,13 +254,36 @@ export async function runDeploy(
     }
 
     if (!forceOnUncommitedChanges && !isCleanGit) {
-      throw new AbortError('Uncommitted changes detected.', null, [
+      let errorMessage = 'Uncommitted changes detected';
+      let changedFiles = undefined;
+
+      const nextSteps = [
         [
-          'Commit your changes before deploying or use the ',
+          'Commit your changes before deploying or use the',
           {command: '--force'},
-          ' flag to deploy with uncommitted changes.',
+          'flag to deploy with uncommitted changes.',
         ],
-      ]);
+      ];
+
+      try {
+        changedFiles = (await execAsync('git status -s', {cwd: root})).stdout;
+      } catch {}
+
+      if (changedFiles) {
+        errorMessage += `:\n\n${changedFiles.trimEnd()}`;
+
+        packageManagers.forEach(({name, lockfile, installCommand}) => {
+          if (changedFiles.includes(lockfile)) {
+            nextSteps.push([
+              `If you are using ${name}, try running`,
+              {command: installCommand},
+              `to avoid changes to ${lockfile}.`,
+            ]);
+          }
+        });
+      }
+
+      throw new AbortError(errorMessage, null, nextSteps);
     }
   }
 

diff --git a/packages/cli/src/lib/check-lockfile.test.ts b/packages/cli/src/lib/check-lockfile.test.ts
@@ -63,6 +63,12 @@ describe('checkLockfileStatus()', () => {
         expect(outputMock.warn()).toMatch(
           / warning .+ Multiple lockfiles found .+/is,
         );
+        expect(outputMock.warn()).toMatch(
+          /package-lock\.json \(created by npm\)/is,
+        );
+        expect(outputMock.warn()).toMatch(
+          /pnpm-lock\.yaml \(created by pnpm\)/is,
+        );
       });
     });
 

diff --git a/packages/cli/src/lib/check-lockfile.ts b/packages/cli/src/lib/check-lockfile.ts
@@ -3,10 +3,7 @@ import {resolvePath} from '@shopify/cli-kit/node/path';
 import {checkIfIgnoredInGitRepository} from '@shopify/cli-kit/node/git';
 import {renderWarning} from '@shopify/cli-kit/node/ui';
 import {AbortError} from '@shopify/cli-kit/node/error';
-import {
-  lockfiles,
-  type Lockfile,
-} from '@shopify/cli-kit/node/node-package-manager';
+import {packageManagers, type PackageManager} from './package-managers.js';
 
 function missingLockfileWarning(shouldExit: boolean) {
   const headline = 'No lockfile found';
@@ -31,16 +28,12 @@ function missingLockfileWarning(shouldExit: boolean) {
   }
 }
 
-function multipleLockfilesWarning(lockfiles: Lockfile[], shouldExit: boolean) {
-  const packageManagers = {
-    'bun.lockb': 'bun',
-    'yarn.lock': 'yarn',
-    'package-lock.json': 'npm',
-    'pnpm-lock.yaml': 'pnpm',
-  };
-
-  const lockfileList = lockfiles.map((lockfile) => {
-    return `${lockfile} (created by ${packageManagers[lockfile]})`;
+function multipleLockfilesWarning(
+  packageManagers: PackageManager[],
+  shouldExit: boolean,
+) {
+  const lockfileList = packageManagers.map(({name, lockfile}) => {
+    return `${lockfile} (created by ${name})`;
   });
 
   const headline = 'Multiple lockfiles found';
@@ -82,22 +75,22 @@ export async function checkLockfileStatus(
 ) {
   if (process.env.LOCAL_DEV) return;
 
-  const availableLockfiles: Lockfile[] = [];
-  for (const lockFileName of lockfiles) {
-    if (await fileExists(resolvePath(directory, lockFileName))) {
-      availableLockfiles.push(lockFileName);
+  const foundPackageManagers: PackageManager[] = [];
+  for (const packageManager of packageManagers) {
+    if (await fileExists(resolvePath(directory, packageManager.lockfile))) {
+      foundPackageManagers.push(packageManager);
     }
   }
 
-  if (availableLockfiles.length === 0) {
+  if (foundPackageManagers.length === 0) {
     return missingLockfileWarning(shouldExit);
   }
 
-  if (availableLockfiles.length > 1) {
-    return multipleLockfilesWarning(availableLockfiles, shouldExit);
+  if (foundPackageManagers.length > 1) {
+    return multipleLockfilesWarning(foundPackageManagers, shouldExit);
   }
 
-  const lockfile = availableLockfiles[0]!;
+  const lockfile = foundPackageManagers[0]!.lockfile;
   const ignoredLockfile = await checkIfIgnoredInGitRepository(directory, [
     lockfile,
   ]).catch(() => {