Merge pull request #6312 from Shopify/jb-multi-env-conditional-sessions

Only create sessions for multi env commands that require authentication

Author: dejmedus

.changeset/large-otters-dream.md

@@ -0,0 +1,6 @@
+---
+'@shopify/cli-kit': minor
+'@shopify/theme': minor
+---
+
+Commands that don't require authentication should not create sessions when ran with multiple environments

packages/theme/src/cli/utilities/theme-command.test.ts

@@ -85,6 +85,32 @@ class TestThemeCommandWithUnionFlags extends TestThemeCommand {
   }
 }
 
+class TestUnauthenticatedThemeCommand extends ThemeCommand {
+  static flags = {
+    environment: Flags.string({
+      multiple: true,
+      default: [],
+      env: 'SHOPIFY_FLAG_ENVIRONMENT',
+    }),
+    store: Flags.string({
+      env: 'SHOPIFY_FLAG_STORE',
+    }),
+  }
+
+  static multiEnvironmentsFlags: RequiredFlags = ['store']
+
+  commandCalls: {flags: any; session: AdminSession; multiEnvironment?: boolean; context?: any}[] = []
+
+  async command(
+    flags: any,
+    session: AdminSession,
+    multiEnvironment?: boolean,
+    context?: {stdout?: Writable; stderr?: Writable},
+  ): Promise<void> {
+    this.commandCalls.push({flags, session, multiEnvironment, context})
+  }
+}
+
 describe('ThemeCommand', () => {
   let mockSession: AdminSession
 
@@ -552,5 +578,31 @@ describe('ThemeCommand', () => {
       expect(liveEnvFlags?.live).toEqual(true)
       expect(liveEnvFlags?.['no-color']).toEqual(true)
     })
+
+    test('commands will only create a session object if the password flag is supported', async () => {
+      // Given
+      vi.mocked(loadEnvironment)
+        .mockResolvedValueOnce({store: 'store1.myshopify.com'})
+        .mockResolvedValueOnce({store: 'store2.myshopify.com'})
+
+      vi.mocked(renderConcurrent).mockImplementation(async ({processes}) => {
+        for (const process of processes) {
+          // eslint-disable-next-line no-await-in-loop
+          await process.action({} as Writable, {} as Writable, {} as any)
+        }
+      })
+
+      await CommandConfig.load()
+      const command = new TestUnauthenticatedThemeCommand(
+        ['--environment', 'store1', '--environment', 'store2'],
+        CommandConfig,
+      )
+
+      // When
+      await command.run()
+
+      // Then
+      expect(ensureAuthenticatedThemes).not.toHaveBeenCalled()
+    })
   })
 })

packages/theme/src/cli/utilities/theme-command.ts

@@ -27,6 +27,11 @@ interface PassThroughFlagsOptions {
   // Only pass on flags that are relevant to CLI2
   allowedFlags?: string[]
 }
+interface ValidEnvironment {
+  environment: EnvironmentName
+  flags: FlagValues
+  requiresAuth: boolean
+}
 type EnvironmentName = string
 /**
  * Flags required to run a command in multiple environments
@@ -67,7 +72,7 @@ export default abstract class ThemeCommand extends Command {
 
   async command(
     _flags: FlagValues,
-    _session: AdminSession,
+    _session?: AdminSession,
     _multiEnvironment = false,
     _context?: {stdout?: Writable; stderr?: Writable},
   ): Promise<void> {}
@@ -84,18 +89,17 @@ export default abstract class ThemeCommand extends Command {
     }
     const requiredFlags = klass.multiEnvironmentsFlags
     const {flags} = await this.parse(klass)
-
+    const commandRequiresAuth = 'password' in klass.flags
     const environments = (Array.isArray(flags.environment) ? flags.environment : [flags.environment]).filter(Boolean)
 
     // Single environment or no environment
     if (environments.length <= 1) {
-      const session = await this.createSession(flags)
+      const session = commandRequiresAuth ? await this.createSession(flags) : undefined
       const commandName = this.constructor.name.toLowerCase()
 
       recordEvent(`theme-command:${commandName}:single-env:authenticated`)
 
       await this.command(flags, session)
-      await this.logAnalyticsData(session)
       return
     }
 
@@ -112,7 +116,7 @@ export default abstract class ThemeCommand extends Command {
     }
 
     const environmentsMap = await this.loadEnvironments(environments, flags, flagsWithoutDefaults)
-    const validationResults = await this.validateEnvironments(environmentsMap, requiredFlags)
+    const validationResults = await this.validateEnvironments(environmentsMap, requiredFlags, commandRequiresAuth)
 
     const commandAllowsForceFlag = 'force' in klass.flags
 
@@ -160,13 +164,15 @@ export default abstract class ThemeCommand extends Command {
    * Split environments into valid and invalid based on flags
    * @param environmentMap - The map of environments to validate
    * @param requiredFlags - The required flags to check for
+   * @param requiresAuth - Whether the command requires authentication
    * @returns An object containing valid and invalid environment arrays
    */
   private async validateEnvironments(
     environmentMap: Map<EnvironmentName, FlagValues>,
     requiredFlags: Exclude<RequiredFlags, null>,
+    requiresAuth: boolean,
   ) {
-    const valid: {environment: EnvironmentName; flags: FlagValues}[] = []
+    const valid: ValidEnvironment[] = []
     const invalid: {environment: EnvironmentName; reason: string}[] = []
 
     for (const [environmentName, environmentFlags] of environmentMap) {
@@ -176,8 +182,7 @@ export default abstract class ThemeCommand extends Command {
         invalid.push({environment: environmentName, reason: `Missing flags: ${missingFlagsText}`})
         continue
       }
-
-      valid.push({environment: environmentName, flags: environmentFlags})
+      valid.push({environment: environmentName, flags: environmentFlags, requiresAuth})
     }
 
     return {valid, invalid}
@@ -194,7 +199,7 @@ export default abstract class ThemeCommand extends Command {
     commandName: string,
     requiredFlags: Exclude<RequiredFlags, null>,
     validationResults: {
-      valid: {environment: string; flags: FlagValues}[]
+      valid: ValidEnvironment[]
       invalid: {environment: string; reason: string}[]
     },
   ) {
@@ -234,7 +239,7 @@ export default abstract class ThemeCommand extends Command {
    * Run the command in each valid environment concurrently
    * @param validEnvironments - The valid environments to run the command in
    */
-  private async runConcurrent(validEnvironments: {environment: EnvironmentName; flags: FlagValues}[]) {
+  private async runConcurrent(validEnvironments: ValidEnvironment[]) {
     const abortController = new AbortController()
 
     const stores = validEnvironments.map((env) => env.flags.store as string)
@@ -245,13 +250,13 @@ export default abstract class ThemeCommand extends Command {
     for (const runGroup of runGroups) {
       // eslint-disable-next-line no-await-in-loop
       await renderConcurrent({
-        processes: runGroup.map(({environment, flags}) => ({
+        processes: runGroup.map(({environment, flags, requiresAuth}) => ({
           prefix: environment,
           action: async (stdout: Writable, stderr: Writable, _signal) => {
             try {
               const store = flags.store as string
               await useThemeStoreContext(store, async () => {
-                const session = await this.createSession(flags)
+                const session = requiresAuth ? await this.createSession(flags) : undefined
 
                 const commandName = this.constructor.name.toLowerCase()
                 recordEvent(`theme-command:${commandName}:multi-env:authenticated`)
@@ -280,8 +285,8 @@ export default abstract class ThemeCommand extends Command {
    * @param environments - The environments to group
    * @returns The environment groups
    */
-  private createSequentialGroups(environments: {environment: EnvironmentName; flags: FlagValues}[]) {
-    const groups: {environment: EnvironmentName; flags: FlagValues}[][] = []
+  private createSequentialGroups(environments: ValidEnvironment[]) {
+    const groups: ValidEnvironment[][] = []
 
     environments.forEach((environment) => {
       const groupWithoutStore = groups.find((arr) => !arr.some((env) => env.flags.store === environment.flags.store))