Merge pull request #4900 from Shopify/jm/strict_push

Add --strict flag to theme push to enforce theme check before pushing

.changeset/chilly-donkeys-exist.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme': minor
+---
+
+Add `--strict` flag to `theme push` command, which will report `theme check` warnings and abort the operation if there are errors.

docs-shopify.dev/commands/interfaces/theme-push.interface.ts

@@ -78,6 +78,12 @@ export interface themepush {
    */
   '-s, --store <value>'?: string
 
+  /**
+   * Require theme check to pass without errors before pushing. Warnings are allowed.
+   * @environment SHOPIFY_FLAG_STRICT_PUSH
+   */
+  '--strict'?: ''
+
   /**
    * Theme ID or name of the remote theme.
    * @environment SHOPIFY_FLAG_THEME_ID

docs-shopify.dev/generated/generated_docs_data.json

@@ -5819,6 +5819,15 @@
                 "isOptional": true,
                 "environmentValue": "SHOPIFY_FLAG_PATH"
               },
+              {
+                "filePath": "docs-shopify.dev/commands/interfaces/theme-push.interface.ts",
+                "syntaxKind": "PropertySignature",
+                "name": "--strict",
+                "value": "\"\"",
+                "description": "Require theme check to pass without errors before pushing. Warnings are allowed.",
+                "isOptional": true,
+                "environmentValue": "SHOPIFY_FLAG_STRICT_PUSH"
+              },
               {
                 "filePath": "docs-shopify.dev/commands/interfaces/theme-push.interface.ts",
                 "syntaxKind": "PropertySignature",
@@ -5937,7 +5946,7 @@
                 "environmentValue": "SHOPIFY_FLAG_IGNORE"
               }
             ],
-            "value": "export interface themepush {\n  /**\n   * Allow push to a live theme.\n   * @environment SHOPIFY_FLAG_ALLOW_LIVE\n   */\n  '-a, --allow-live'?: ''\n\n  /**\n   * Push theme files from your remote development theme.\n   * @environment SHOPIFY_FLAG_DEVELOPMENT\n   */\n  '-d, --development'?: ''\n\n  /**\n   * The environment to apply to the current command.\n   * @environment SHOPIFY_FLAG_ENVIRONMENT\n   */\n  '-e, --environment <value>'?: string\n\n  /**\n   * Skip downloading the specified files (Multiple flags allowed).\n   * @environment SHOPIFY_FLAG_IGNORE\n   */\n  '-x, --ignore <value>'?: string\n\n  /**\n   * Output the result as JSON.\n   * @environment SHOPIFY_FLAG_JSON\n   */\n  '-j, --json'?: ''\n\n  /**\n   * Push theme files from your remote live theme.\n   * @environment SHOPIFY_FLAG_LIVE\n   */\n  '-l, --live'?: ''\n\n  /**\n   * Disable color output.\n   * @environment SHOPIFY_FLAG_NO_COLOR\n   */\n  '--no-color'?: ''\n\n  /**\n   * Prevent deleting remote files that don't exist locally.\n   * @environment SHOPIFY_FLAG_NODELETE\n   */\n  '-n, --nodelete'?: ''\n\n  /**\n   * Download only the specified files (Multiple flags allowed).\n   * @environment SHOPIFY_FLAG_ONLY\n   */\n  '-o, --only <value>'?: string\n\n  /**\n   * Password generated from the Theme Access app.\n   * @environment SHOPIFY_CLI_THEME_TOKEN\n   */\n  '--password <value>'?: string\n\n  /**\n   * The path to your theme directory.\n   * @environment SHOPIFY_FLAG_PATH\n   */\n  '--path <value>'?: string\n\n  /**\n   * Publish as the live theme after uploading.\n   * @environment SHOPIFY_FLAG_PUBLISH\n   */\n  '-p, --publish'?: ''\n\n  /**\n   * Store URL. It can be the store prefix (example) or the full myshopify.com URL (example.myshopify.com, https://example.myshopify.com).\n   * @environment SHOPIFY_FLAG_STORE\n   */\n  '-s, --store <value>'?: string\n\n  /**\n   * Theme ID or name of the remote theme.\n   * @environment SHOPIFY_FLAG_THEME_ID\n   */\n  '-t, --theme <value>'?: string\n\n  /**\n   * Create a new unpublished theme and push to it.\n   * @environment SHOPIFY_FLAG_UNPUBLISHED\n   */\n  '-u, --unpublished'?: ''\n\n  /**\n   * Increase the verbosity of the output.\n   * @environment SHOPIFY_FLAG_VERBOSE\n   */\n  '--verbose'?: ''\n}"
+            "value": "export interface themepush {\n  /**\n   * Allow push to a live theme.\n   * @environment SHOPIFY_FLAG_ALLOW_LIVE\n   */\n  '-a, --allow-live'?: ''\n\n  /**\n   * Push theme files from your remote development theme.\n   * @environment SHOPIFY_FLAG_DEVELOPMENT\n   */\n  '-d, --development'?: ''\n\n  /**\n   * The environment to apply to the current command.\n   * @environment SHOPIFY_FLAG_ENVIRONMENT\n   */\n  '-e, --environment <value>'?: string\n\n  /**\n   * Skip downloading the specified files (Multiple flags allowed).\n   * @environment SHOPIFY_FLAG_IGNORE\n   */\n  '-x, --ignore <value>'?: string\n\n  /**\n   * Output the result as JSON.\n   * @environment SHOPIFY_FLAG_JSON\n   */\n  '-j, --json'?: ''\n\n  /**\n   * Push theme files from your remote live theme.\n   * @environment SHOPIFY_FLAG_LIVE\n   */\n  '-l, --live'?: ''\n\n  /**\n   * Disable color output.\n   * @environment SHOPIFY_FLAG_NO_COLOR\n   */\n  '--no-color'?: ''\n\n  /**\n   * Prevent deleting remote files that don't exist locally.\n   * @environment SHOPIFY_FLAG_NODELETE\n   */\n  '-n, --nodelete'?: ''\n\n  /**\n   * Download only the specified files (Multiple flags allowed).\n   * @environment SHOPIFY_FLAG_ONLY\n   */\n  '-o, --only <value>'?: string\n\n  /**\n   * Password generated from the Theme Access app.\n   * @environment SHOPIFY_CLI_THEME_TOKEN\n   */\n  '--password <value>'?: string\n\n  /**\n   * The path to your theme directory.\n   * @environment SHOPIFY_FLAG_PATH\n   */\n  '--path <value>'?: string\n\n  /**\n   * Publish as the live theme after uploading.\n   * @environment SHOPIFY_FLAG_PUBLISH\n   */\n  '-p, --publish'?: ''\n\n  /**\n   * Store URL. It can be the store prefix (example) or the full myshopify.com URL (example.myshopify.com, https://example.myshopify.com).\n   * @environment SHOPIFY_FLAG_STORE\n   */\n  '-s, --store <value>'?: string\n\n  /**\n   * Require theme check to pass without errors before pushing. Warnings are allowed.\n   * @environment SHOPIFY_FLAG_STRICT_PUSH\n   */\n  '--strict'?: ''\n\n  /**\n   * Theme ID or name of the remote theme.\n   * @environment SHOPIFY_FLAG_THEME_ID\n   */\n  '-t, --theme <value>'?: string\n\n  /**\n   * Create a new unpublished theme and push to it.\n   * @environment SHOPIFY_FLAG_UNPUBLISHED\n   */\n  '-u, --unpublished'?: ''\n\n  /**\n   * Increase the verbosity of the output.\n   * @environment SHOPIFY_FLAG_VERBOSE\n   */\n  '--verbose'?: ''\n}"
           }
         }
       }

packages/cli/README.md

@@ -2118,6 +2118,7 @@ FLAGS
       --no-color             Disable color output.
       --password=<value>     Password generated from the Theme Access app.
       --path=<value>         The path to your theme directory.
+      --strict               Require theme check to pass without errors before pushing. Warnings are allowed.
       --verbose              Increase the verbosity of the output.
 
 DESCRIPTION

packages/cli/oclif.manifest.json

@@ -5791,6 +5791,13 @@
           "name": "store",
           "type": "option"
         },
+        "strict": {
+          "allowNo": false,
+          "description": "Require theme check to pass without errors before pushing. Warnings are allowed.",
+          "env": "SHOPIFY_FLAG_STRICT_PUSH",
+          "name": "strict",
+          "type": "boolean"
+        },
         "theme": {
           "char": "t",
           "description": "Theme ID or name of the remote theme.",

packages/theme/src/cli/commands/theme/check.ts

@@ -134,52 +134,58 @@ export default class Check extends ThemeCommand {
       return
     }
 
-    const {offenses, theme} = await themeCheckRun(path, config, (message) => {
-      // We should replace this with outputDebug when it logs to STDERR by default.
-      // We need this right now because the --verbose flags pollutes STDOUT
-      // when used with --output=json with the current outputDebug defaults.
-      if (process.env.SHOPIFY_TMP_FLAG_DEBUG) {
-        consoleError(message)
-      }
-    })
-
-    const offensesByFile = sortOffenses(offenses)
+    const {offenses, theme} = await runThemeCheck(path, flags.output, config)
 
-    if (flags.output === 'text') {
-      renderOffensesText(offensesByFile, path)
+    if (flags['auto-correct']) {
+      await performAutoFixes(theme, offenses)
+    }
 
-      // Use renderSuccess when theres no offenses
-      const render = offenses.length ? renderInfo : renderSuccess
+    return handleExit(offenses, flags['fail-level'] as FailLevel)
+  }
+}
 
-      render({
-        headline: 'Theme Check Summary.',
-        body: formatSummary(offenses, offensesByFile, theme),
-      })
+export async function runThemeCheck(path: string, outputFormat: string, config?: string) {
+  const {offenses, theme} = await themeCheckRun(path, config, (message) => {
+    // We should replace this with outputDebug when it logs to STDERR by default.
+    // We need this right now because the --verbose flags pollutes STDOUT
+    // when used with --output=json with the current outputDebug defaults.
+    if (process.env.SHOPIFY_TMP_FLAG_DEBUG) {
+      consoleError(message)
     }
+  })
 
-    if (flags.output === 'json') {
-      /**
-       * Workaround:
-       * Force stdout to be blocking so that the JSON output is not broken when piped to another process.
-       * ie: ` | jq .`
-       * It turns out that console.log is technically asynchronous, and when we call process.exit(),
-       * node doesn't wait on all the output being sent to stdout and instead closes the process immediately
-       *
-       * https://github.com/pnp/cli-microsoft365/issues/1266#issuecomment-727254264
-       *
-       */
-      const stdout = process.stdout
-      if (isExtendedWriteStream(stdout)) {
-        stdout._handle.setBlocking(true)
-      }
+  const offensesByFile = sortOffenses(offenses)
 
-      outputInfo(JSON.stringify(formatOffensesJson(offensesByFile)))
-    }
+  if (outputFormat === 'text') {
+    renderOffensesText(offensesByFile, path)
 
-    if (flags['auto-correct']) {
-      await performAutoFixes(theme, offenses)
+    // Use renderSuccess when theres no offenses
+    const render = offenses.length ? renderInfo : renderSuccess
+
+    render({
+      headline: 'Theme Check Summary.',
+      body: formatSummary(offenses, offensesByFile, theme),
+    })
+  }
+
+  if (outputFormat === 'json') {
+    /**
+     * Workaround:
+     * Force stdout to be blocking so that the JSON output is not broken when piped to another process.
+     * ie: ` | jq .`
+     * It turns out that console.log is technically asynchronous, and when we call process.exit(),
+     * node doesn't wait on all the output being sent to stdout and instead closes the process immediately
+     *
+     * https://github.com/pnp/cli-microsoft365/issues/1266#issuecomment-727254264
+     *
+     */
+    const stdout = process.stdout
+    if (isExtendedWriteStream(stdout)) {
+      stdout._handle.setBlocking(true)
     }
 
-    return handleExit(offenses, flags['fail-level'] as FailLevel)
+    outputInfo(JSON.stringify(formatOffensesJson(offensesByFile)))
   }
+
+  return {offenses, theme}
 }

packages/theme/src/cli/commands/theme/push.ts

@@ -97,6 +97,10 @@ export default class Push extends ThemeCommand {
       description: 'Proceed without confirmation, if current directory does not seem to be theme directory.',
       env: 'SHOPIFY_FLAG_FORCE',
     }),
+    strict: Flags.boolean({
+      description: 'Require theme check to pass without errors before pushing. Warnings are allowed.',
+      env: 'SHOPIFY_FLAG_STRICT_PUSH',
+    }),
   }
 
   async run(): Promise<void> {
@@ -120,6 +124,7 @@ export default class Push extends ThemeCommand {
       force: flags.force,
       noColor: flags['no-color'],
       verbose: flags.verbose,
+      strict: flags.strict,
     }
 
     await push(pushFlags)

packages/theme/src/cli/services/push.test.ts

@@ -4,6 +4,7 @@ import {setDevelopmentTheme} from './local-storage.js'
 import {uploadTheme} from '../utilities/theme-uploader.js'
 import {ensureThemeStore} from '../utilities/theme-store.js'
 import {findOrSelectTheme} from '../utilities/theme-selector.js'
+import {runThemeCheck} from '../commands/theme/check.js'
 import {buildTheme} from '@shopify/cli-kit/node/themes/factories'
 import {test, describe, vi, expect, beforeEach} from 'vitest'
 import {createTheme, fetchTheme, themePublish} from '@shopify/cli-kit/node/themes/api'
@@ -15,6 +16,8 @@ import {
   UNPUBLISHED_THEME_ROLE,
 } from '@shopify/cli-kit/node/themes/utils'
 import {renderConfirmationPrompt} from '@shopify/cli-kit/node/ui'
+import {AbortError} from '@shopify/cli-kit/node/error'
+import {Severity, SourceCodeType} from '@shopify/theme-check-node'
 
 vi.mock('../utilities/theme-uploader.js')
 vi.mock('../utilities/theme-store.js')
@@ -24,6 +27,7 @@ vi.mock('@shopify/cli-kit/node/themes/utils')
 vi.mock('@shopify/cli-kit/node/session')
 vi.mock('@shopify/cli-kit/node/themes/api')
 vi.mock('@shopify/cli-kit/node/ui')
+vi.mock('../commands/theme/check.js')
 
 const path = '/my-theme'
 const defaultFlags: PullFlags = {
@@ -59,6 +63,129 @@ describe('push', () => {
     // Then
     expect(themePublish).toHaveBeenCalledWith(theme.id, adminSession)
   })
+
+  describe('strict mode', () => {
+    beforeEach(() => {
+      const theme = buildTheme({id: 1, name: 'Theme', role: 'development'})!
+      vi.mocked(findOrSelectTheme).mockResolvedValue(theme)
+    })
+
+    test('skips theme check when strict mode is disabled', async () => {
+      // Given
+      const flags = {...defaultFlags, strict: false}
+
+      // When
+      await push(flags)
+
+      // Then
+      expect(runThemeCheck).not.toHaveBeenCalled()
+    })
+
+    test('blocks push when errors exist', async () => {
+      // Given
+      vi.mocked(runThemeCheck).mockResolvedValue({
+        offenses: [
+          {
+            severity: Severity.ERROR,
+            message: 'error message',
+            type: SourceCodeType.LiquidHtml,
+            check: 'check',
+            absolutePath: '/path/to/file.liquid',
+            start: {index: 0, line: 1, character: 1},
+            end: {index: 1, line: 1, character: 1},
+          },
+        ],
+        theme: [],
+      })
+
+      // When/Then
+      await expect(push({...defaultFlags, strict: true})).rejects.toThrow(AbortError)
+    })
+
+    test('blocks push when both warnings and errors exist', async () => {
+      // Given
+      vi.mocked(runThemeCheck).mockResolvedValue({
+        offenses: [
+          {
+            severity: Severity.WARNING,
+            message: 'warning message',
+            type: SourceCodeType.LiquidHtml,
+            check: 'check',
+            absolutePath: '/path/to/file.liquid',
+            start: {index: 0, line: 1, character: 1},
+            end: {index: 1, line: 1, character: 1},
+          },
+          {
+            severity: Severity.ERROR,
+            message: 'error message',
+            type: SourceCodeType.LiquidHtml,
+            check: 'check',
+            absolutePath: '/path/to/file.liquid',
+            start: {index: 0, line: 1, character: 1},
+            end: {index: 1, line: 1, character: 1},
+          },
+        ],
+        theme: [],
+      })
+
+      // When/Then
+      await expect(push({...defaultFlags, strict: true})).rejects.toThrow(AbortError)
+    })
+
+    test('continues push when no offenses exist', async () => {
+      // Given
+      vi.mocked(runThemeCheck).mockResolvedValue({
+        offenses: [],
+        theme: [],
+      })
+
+      // When/Then
+      await expect(push({...defaultFlags, strict: true})).resolves.not.toThrow()
+    })
+
+    test('continues push when only warnings exist', async () => {
+      // Given
+      vi.mocked(runThemeCheck).mockResolvedValue({
+        offenses: [
+          {
+            severity: Severity.WARNING,
+            message: 'warning message',
+            check: 'check',
+            absolutePath: '/path/to/file.liquid',
+            type: SourceCodeType.LiquidHtml,
+            start: {index: 0, line: 1, character: 1},
+            end: {index: 1, line: 1, character: 1},
+          },
+        ],
+        theme: [],
+      })
+
+      // When/Then
+      await expect(push({...defaultFlags, strict: true})).resolves.not.toThrow()
+    })
+
+    test('passes the --json flag to theme check as output format', async () => {
+      // Given
+      vi.mocked(runThemeCheck).mockResolvedValue({
+        offenses: [
+          {
+            severity: Severity.WARNING,
+            message: 'warning message',
+            check: 'check',
+            absolutePath: '/path/to/file.liquid',
+            type: SourceCodeType.LiquidHtml,
+            start: {index: 0, line: 1, character: 1},
+            end: {index: 1, line: 1, character: 1},
+          },
+        ],
+        theme: [],
+      })
+
+      // When/Then
+      await push({...defaultFlags, strict: true, json: true})
+      expect(runThemeCheck).toHaveBeenCalledWith(path, 'json')
+    })
+  })
 })
 
 describe('createOrSelectTheme', async () => {

packages/theme/src/cli/services/push.ts

@@ -7,6 +7,7 @@ import {DevelopmentThemeManager} from '../utilities/development-theme-manager.js
 import {findOrSelectTheme} from '../utilities/theme-selector.js'
 import {Role} from '../utilities/theme-selector/fetch.js'
 import {configureCLIEnvironment} from '../utilities/cli-config.js'
+import {runThemeCheck} from '../commands/theme/check.js'
 import {AdminSession, ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
 import {createTheme, fetchChecksums, themePublish} from '@shopify/cli-kit/node/themes/api'
 import {Result, Theme} from '@shopify/cli-kit/node/themes/types'
@@ -20,6 +21,8 @@ import {
 import {themeEditorUrl, themePreviewUrl} from '@shopify/cli-kit/node/themes/urls'
 import {cwd, resolvePath} from '@shopify/cli-kit/node/path'
 import {LIVE_THEME_ROLE, promptThemeName, UNPUBLISHED_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils'
+import {AbortError} from '@shopify/cli-kit/node/error'
+import {Severity} from '@shopify/theme-check-node'
 
 interface PushOptions {
   path: string
@@ -94,6 +97,9 @@ export interface PushFlags {
 
   /** Increase the verbosity of the output. */
   verbose?: boolean
+
+  /** Require theme check to pass without errors before pushing. Warnings are allowed. */
+  strict?: boolean
 }
 
 /**
@@ -102,6 +108,18 @@ export interface PushFlags {
  * @param flags - The flags for the push operation.
  */
 export async function push(flags: PushFlags): Promise<void> {
+  if (flags.strict) {
+    const outputType = flags.json ? 'json' : 'text'
+    const {offenses} = await runThemeCheck(flags.path ?? cwd(), outputType)
+
+    if (offenses.length > 0) {
+      const errorOffenses = offenses.filter((offense) => offense.severity === Severity.ERROR)
+      if (errorOffenses.length > 0) {
+        throw new AbortError('Theme check failed. Please fix the errors before pushing.')
+      }
+    }
+  }
+
   const {path} = flags
 
   configureCLIEnvironment({