Throw build errors when lockfile is not valid on CI (#1370)

* Optionally throw errors when checking lockfiles * Throw in CI by default * Fix package-lock * Do not throw when lockfile is git-ignored on CI * Add flag to skip lockfile check * Changesets * Build templates with --no-lockfile-check * Add env variable alias for --no-lockfile-check * Fix no-lockfile-check in monorepo * Fix tests * Debug Oxygen action * Revert "Debug Oxygen action" This reverts commit 79e7f44.
Shopify · Sep 27, 2023 · e148cfc · e148cfc
1 parent bfb142e
commit e148cfc
Show file tree

Hide file tree

Showing 10 changed files with 150 additions and 80 deletions.
diff --git a/.changeset/fresh-cobras-call.md b/.changeset/fresh-cobras-call.md
@@ -0,0 +1,7 @@
+---
+'@shopify/cli-hydrogen': minor
+---
+
+The build command now throws errors on CI when it can't find a valid lockfile. This should prevent unforseen issues related to dependency versioning in production.
+
+This behavior can be disabled with the flag `--no-lockfile-check`, which might be useful in monorepos or other setups where the lockfile is not available in the project directory.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -84,8 +84,8 @@ jobs:
       #     restore-keys: |
       #       turbo-${{ github.job }}-${{ github.ref_name }}-
 
-      - name: 📦 Build packages
-        run: npm run build
+      - name: 📦 Build packages, templates, and examples
+        run: SHOPIFY_HYDROGEN_FLAG_LOCKFILE_CHECK=false npm run build:all
 
       - name: ✅ Typecheck
         run: npm run typecheck
@@ -114,8 +114,8 @@ jobs:
       - name: 📥 Install dependencies
         run: npm ci
 
-      - name: 📦 Build packages
-        run: npm run build
+      - name: 📦 Build packages, templates, and examples
+        run: SHOPIFY_HYDROGEN_FLAG_LOCKFILE_CHECK=false npm run build:all
 
       - name: 🔬 Test
         run: npm run test
diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml
@@ -54,7 +54,7 @@ jobs:
           oxygen_deployment_token: ${{ secrets.OXYGEN_DEPLOYMENT_TOKEN_28966968 }}
           oxygen_worker_dir: dist/worker
           oxygen_client_dir: dist/client
-          build_command: 'HYDROGEN_ASSET_BASE_URL=$OXYGEN_ASSET_BASE_URL npm run build'
+          build_command: 'HYDROGEN_ASSET_BASE_URL=$OXYGEN_ASSET_BASE_URL npm run build -- --no-lockfile-check'
           # Hardcode message and timestamp if manual dispatch
           commit_message: ${{ github.event.head_commit.message || 'Manual deployment' }}
           commit_timestamp: ${{ github.event.head_commit.timestamp || github.event.repository.updated_at }}

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -2,8 +2,9 @@
   "private": true,
   "sideEffects": false,
   "scripts": {
-    "build": "turbo build --parallel",
-    "build:pkg": "npm run build -- --filter=./packages/*",
+    "build": "npm run build:pkg",
+    "build:pkg": "turbo build --parallel --filter=./packages/*",
+    "build:all": "turbo build --parallel",
     "ci:checks": "turbo run lint test format:check typecheck",
     "dev": "npm run dev:pkg",
     "dev:pkg": "cross-env LOCAL_DEV=true turbo dev --parallel --filter=./packages/*",

diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json
@@ -28,6 +28,12 @@
           "description": "Show a bundle size summary after building.",
           "allowNo": true
         },
+        "lockfile-check": {
+          "name": "lockfile-check",
+          "type": "boolean",
+          "description": "Checks that there is exactly 1 valid lockfile in the project.",
+          "allowNo": true
+        },
         "disable-route-warning": {
           "name": "disable-route-warning",
           "type": "boolean",

diff --git a/packages/cli/src/commands/hydrogen/build.ts b/packages/cli/src/commands/hydrogen/build.ts
@@ -37,6 +37,7 @@ import {
   hasMetafile,
 } from '../../lib/bundle/analyzer.js';
 import {AbortError} from '@shopify/cli-kit/node/error';
+import {isCI} from '../../lib/is-ci.js';
 
 const LOG_WORKER_BUILT = '📦 Worker built';
 const MAX_WORKER_BUNDLE_SIZE = 10;
@@ -51,22 +52,29 @@ export default class Build extends Command {
       allowNo: true,
       default: true,
     }),
-    ['bundle-stats']: Flags.boolean({
+    'bundle-stats': Flags.boolean({
       description: 'Show a bundle size summary after building.',
       default: true,
       allowNo: true,
     }),
+    'lockfile-check': Flags.boolean({
+      description:
+        'Checks that there is exactly 1 valid lockfile in the project.',
+      env: 'SHOPIFY_HYDROGEN_FLAG_LOCKFILE_CHECK',
+      default: true,
+      allowNo: true,
+    }),
     'disable-route-warning': Flags.boolean({
       description: 'Disable warning about missing standard routes.',
       env: 'SHOPIFY_HYDROGEN_FLAG_DISABLE_ROUTE_WARNING',
     }),
-    ['codegen-unstable']: Flags.boolean({
+    'codegen-unstable': Flags.boolean({
       description:
         'Generate types for the Storefront API queries found in your project.',
       required: false,
       default: false,
     }),
-    ['codegen-config-path']: commonFlags.codegenConfigPath,
+    'codegen-config-path': commonFlags.codegenConfigPath,
 
     base: deprecated('--base')(),
     entry: deprecated('--entry')(),
@@ -85,23 +93,27 @@ export default class Build extends Command {
   }
 }
 
+type RunBuildOptions = {
+  directory?: string;
+  useCodegen?: boolean;
+  codegenConfigPath?: string;
+  sourcemap?: boolean;
+  disableRouteWarning?: boolean;
+  assetPath?: string;
+  bundleStats?: boolean;
+  lockfileCheck?: boolean;
+};
+
 export async function runBuild({
   directory,
   useCodegen = false,
   codegenConfigPath,
   sourcemap = false,
   disableRouteWarning = false,
   bundleStats = true,
+  lockfileCheck = true,
   assetPath,
-}: {
-  directory?: string;
-  useCodegen?: boolean;
-  codegenConfigPath?: string;
-  sourcemap?: boolean;
-  disableRouteWarning?: boolean;
-  assetPath?: string;
-  bundleStats?: boolean;
-}) {
+}: RunBuildOptions) {
   if (!process.env.NODE_ENV) {
     process.env.NODE_ENV = 'production';
   }
@@ -112,7 +124,11 @@ export async function runBuild({
   const {root, buildPath, buildPathClient, buildPathWorkerFile, publicPath} =
     getProjectPaths(directory);
 
-  await Promise.all([checkLockfileStatus(root), muteRemixLogs()]);
+  if (lockfileCheck) {
+    await checkLockfileStatus(root, isCI());
+  }
+
+  await muteRemixLogs();
 
   console.time(LOG_WORKER_BUILT);
 

diff --git a/packages/cli/src/lib/check-lockfile.test.ts b/packages/cli/src/lib/check-lockfile.test.ts
@@ -65,6 +65,17 @@ describe('checkLockfileStatus()', () => {
         );
       });
     });
+
+    it('throws when shouldExit is true', async () => {
+      await inTemporaryDirectory(async (tmpDir) => {
+        await writeFile(joinPath(tmpDir, 'package-lock.json'), '');
+        await writeFile(joinPath(tmpDir, 'pnpm-lock.yaml'), '');
+
+        await expect(checkLockfileStatus(tmpDir, true)).rejects.toThrow(
+          /Multiple lockfiles found/is,
+        );
+      });
+    });
   });
 
   describe('when a lockfile is missing', () => {
@@ -75,5 +86,13 @@ describe('checkLockfileStatus()', () => {
         expect(outputMock.warn()).toMatch(/ warning .+ No lockfile found .+/is);
       });
     });
+
+    it('throws when shouldExit is true', async () => {
+      await inTemporaryDirectory(async (tmpDir) => {
+        await expect(checkLockfileStatus(tmpDir, true)).rejects.toThrow(
+          /No lockfile found/is,
+        );
+      });
+    });
   });
 });
diff --git a/packages/cli/src/lib/check-lockfile.ts b/packages/cli/src/lib/check-lockfile.ts
@@ -2,31 +2,36 @@ import {fileExists} from '@shopify/cli-kit/node/fs';
 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';
 
-function missingLockfileWarning() {
-  renderWarning({
-    headline: 'No lockfile found',
-    body:
-      `If you don’t commit a lockfile, then your app might install the wrong ` +
-      `package versions when deploying. To avoid versioning issues, generate a ` +
-      `new lockfile and commit it to your repository.`,
-    nextSteps: [
-      [
-        'Generate a lockfile. Run',
-        {
-          command: 'npm|yarn|pnpm install',
-        },
-      ],
-      'Commit the new file to your repository',
+function missingLockfileWarning(shouldExit: boolean) {
+  const headline = 'No lockfile found';
+  const body =
+    `If you don’t commit a lockfile, then your app might install the wrong ` +
+    `package versions when deploying. To avoid versioning issues, generate a ` +
+    `new lockfile and commit it to your repository.`;
+  const nextSteps = [
+    [
+      'Generate a lockfile. Run',
+      {
+        command: 'npm|yarn|pnpm install',
+      },
     ],
-  });
+    'Commit the new file to your repository',
+  ];
+
+  if (shouldExit) {
+    throw new AbortError(headline, body, nextSteps);
+  } else {
+    renderWarning({headline, body, nextSteps});
+  }
 }
 
-function multipleLockfilesWarning(lockfiles: Lockfile[]) {
+function multipleLockfilesWarning(lockfiles: Lockfile[], shouldExit: boolean) {
   const packageManagers = {
     'yarn.lock': 'yarn',
     'package-lock.json': 'npm',
@@ -37,36 +42,43 @@ function multipleLockfilesWarning(lockfiles: Lockfile[]) {
     return `${lockfile} (created by ${packageManagers[lockfile]})`;
   });
 
-  renderWarning({
-    headline: 'Multiple lockfiles found',
-    body: [
-      `Your project contains more than one lockfile. This can cause version ` +
-        `conflicts when installing and deploying your app. The following ` +
-        `lockfiles were detected:\n`,
-      {list: {items: lockfileList}},
-    ],
-    nextSteps: [
-      'Delete any unneeded lockfiles',
-      'Commit the change to your repository',
-    ],
-  });
+  const headline = 'Multiple lockfiles found';
+  const body = [
+    `Your project contains more than one lockfile. This can cause version ` +
+      `conflicts when installing and deploying your app. The following ` +
+      `lockfiles were detected:\n`,
+    {list: {items: lockfileList}},
+  ];
+  const nextSteps = [
+    'Delete any unneeded lockfiles',
+    'Commit the change to your repository',
+  ];
+
+  if (shouldExit) {
+    throw new AbortError(headline, body, nextSteps);
+  } else {
+    renderWarning({headline, body, nextSteps});
+  }
 }
 
 function lockfileIgnoredWarning(lockfile: string) {
-  renderWarning({
-    headline: 'Lockfile ignored by Git',
-    body:
-      `Your project’s lockfile isn’t being tracked by Git. If you don’t commit ` +
-      `a lockfile, then your app might install the wrong package versions when ` +
-      `deploying.`,
-    nextSteps: [
-      `In your project’s .gitignore file, delete any references to ${lockfile}`,
-      'Commit the change to your repository',
-    ],
-  });
+  const headline = 'Lockfile ignored by Git';
+  const body =
+    `Your project’s lockfile isn’t being tracked by Git. If you don’t commit ` +
+    `a lockfile, then your app might install the wrong package versions when ` +
+    `deploying.`;
+  const nextSteps = [
+    `In your project’s .gitignore file, delete any references to ${lockfile}`,
+    'Commit the change to your repository',
+  ];
+
+  renderWarning({headline, body, nextSteps});
 }
 
-export async function checkLockfileStatus(directory: string) {
+export async function checkLockfileStatus(
+  directory: string,
+  shouldExit = false,
+) {
   if (process.env.LOCAL_DEV) return;
 
   const availableLockfiles: Lockfile[] = [];
@@ -76,24 +88,23 @@ export async function checkLockfileStatus(directory: string) {
     }
   }
 
-  if (!availableLockfiles.length) {
-    return missingLockfileWarning();
+  if (availableLockfiles.length === 0) {
+    return missingLockfileWarning(shouldExit);
   }
 
   if (availableLockfiles.length > 1) {
-    return multipleLockfilesWarning(availableLockfiles);
+    return multipleLockfilesWarning(availableLockfiles, shouldExit);
   }
 
-  try {
-    const lockfile = availableLockfiles[0]!;
-    const ignoredLockfile = await checkIfIgnoredInGitRepository(directory, [
-      lockfile,
-    ]);
-
-    if (ignoredLockfile.length) {
-      lockfileIgnoredWarning(lockfile);
-    }
-  } catch {
+  const lockfile = availableLockfiles[0]!;
+  const ignoredLockfile = await checkIfIgnoredInGitRepository(directory, [
+    lockfile,
+  ]).catch(() => {
     // Not a Git repository, ignore
+    return [];
+  });
+
+  if (ignoredLockfile.length > 0) {
+    lockfileIgnoredWarning(lockfile);
   }
 }