From e148cfca004d6bb2981d136231e3825509d52305 Mon Sep 17 00:00:00 2001 From: Fran Dios Date: Wed, 27 Sep 2023 18:39:15 +0900 Subject: [PATCH] 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 79e7f441578d46d6be0402207d4a7826e631b74d. --- .changeset/fresh-cobras-call.md | 7 ++ .github/workflows/ci.yml | 8 +- .github/workflows/deploy.yml | 2 +- package-lock.json | 10 +- package.json | 5 +- packages/cli/oclif.manifest.json | 6 + packages/cli/src/commands/hydrogen/build.ts | 42 ++++--- packages/cli/src/lib/check-lockfile.test.ts | 19 +++ packages/cli/src/lib/check-lockfile.ts | 121 +++++++++++--------- packages/cli/src/lib/is-ci.ts | 10 ++ 10 files changed, 150 insertions(+), 80 deletions(-) create mode 100644 .changeset/fresh-cobras-call.md create mode 100644 packages/cli/src/lib/is-ci.ts diff --git a/.changeset/fresh-cobras-call.md b/.changeset/fresh-cobras-call.md new file mode 100644 index 0000000000..a08377d696 --- /dev/null +++ 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 index cdb10e77dd..303e8e0a3f 100644 --- 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 index 77492b7802..030e22edb5 100644 --- 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 index 7028a4d5c5..21426ccb8c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29088,7 +29088,7 @@ }, "packages/cli": { "name": "@shopify/cli-hydrogen", - "version": "5.3.0", + "version": "5.3.1", "license": "MIT", "dependencies": { "@ast-grep/napi": "0.11.0", @@ -29688,7 +29688,7 @@ "dependencies": { "@remix-run/react": "1.19.1", "@shopify/cli": "3.49.2", - "@shopify/cli-hydrogen": "^5.3.0", + "@shopify/cli-hydrogen": "^5.3.1", "@shopify/hydrogen": "^2023.7.8", "@shopify/remix-oxygen": "^1.1.4", "@total-typescript/ts-reset": "^0.4.2", @@ -29719,7 +29719,7 @@ "dependencies": { "@remix-run/react": "1.19.1", "@shopify/cli": "3.49.2", - "@shopify/cli-hydrogen": "^5.3.0", + "@shopify/cli-hydrogen": "^5.3.1", "@shopify/hydrogen": "^2023.7.8", "@shopify/remix-oxygen": "^1.1.4", "graphql": "^16.6.0", @@ -39653,7 +39653,7 @@ "@remix-run/dev": "1.19.1", "@remix-run/react": "1.19.1", "@shopify/cli": "3.49.2", - "@shopify/cli-hydrogen": "^5.3.0", + "@shopify/cli-hydrogen": "^5.3.1", "@shopify/hydrogen": "^2023.7.8", "@shopify/oxygen-workers-types": "^3.17.3", "@shopify/prettier-config": "^1.1.2", @@ -46483,7 +46483,7 @@ "@remix-run/dev": "1.19.1", "@remix-run/react": "1.19.1", "@shopify/cli": "3.49.2", - "@shopify/cli-hydrogen": "^5.3.0", + "@shopify/cli-hydrogen": "^5.3.1", "@shopify/hydrogen": "^2023.7.8", "@shopify/oxygen-workers-types": "^3.17.3", "@shopify/prettier-config": "^1.1.2", diff --git a/package.json b/package.json index b07e6f15ee..73df76e0d0 100644 --- 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 index 994b4b86d5..1fad743015 100644 --- 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 index 456b3a2f1c..c7df1fdf3b 100644 --- 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,6 +93,17 @@ 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, @@ -92,16 +111,9 @@ export async function runBuild({ 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 index 050bba5d48..3e92cfb752 100644 --- 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 index f5aa57cbeb..9310084089 100644 --- 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); } } diff --git a/packages/cli/src/lib/is-ci.ts b/packages/cli/src/lib/is-ci.ts new file mode 100644 index 0000000000..0ded0d72c3 --- /dev/null +++ b/packages/cli/src/lib/is-ci.ts @@ -0,0 +1,10 @@ +/** + * Detects if the process is running in a CI environment. + */ +export function isCI() { + const {env} = process; + + return env.CI === 'false' + ? false // Overrides + : !!(env.CI || env.CI_NAME || env.BUILD_NUMBER || env.TF_BUILD); +}