Run environments acting on the same store sequentially

Previously, if multiple environments were acting on
the same store during multi environment command calls
there was a chance that commands would behave strangely
due to concurrent process interleaving

For example: pushing a dev theme to the same store
twice should check if there is already a dev theme
to push to and only if not, create a new one. However,
during concurrent execution, two new themes were being
created since both environments local storage READs
occurred before either's WRITEs

This commit groups environments with unique store
URLs and runs those groups sequentially, preventing
this behaviour

Author: dejmedus

.changeset/many-ties-check.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme': minor
+---
+
+Allow commands run in multiple environments to act on the same store URL

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

@@ -135,8 +135,9 @@ describe('ThemeCommand', () => {
 
     test('multiple environments provided - uses renderConcurrent for parallel execution', async () => {
       // Given
-      const environmentConfig = {store: 'store.myshopify.com'}
-      vi.mocked(loadEnvironment).mockResolvedValue(environmentConfig)
+      vi.mocked(loadEnvironment)
+        .mockResolvedValueOnce({store: 'store1.myshopify.com', development: true})
+        .mockResolvedValueOnce({store: 'store2.myshopify.com', theme: 'staging'})
       vi.mocked(ensureAuthenticatedThemes).mockResolvedValue(mockSession)
 
       vi.mocked(renderConcurrent).mockResolvedValue(undefined)
@@ -150,7 +151,6 @@ describe('ThemeCommand', () => {
       // Then
       expect(loadEnvironment).toHaveBeenCalledWith('development', 'shopify.theme.toml', {from: undefined, silent: true})
       expect(loadEnvironment).toHaveBeenCalledWith('staging', 'shopify.theme.toml', {from: undefined, silent: true})
-      expect(ensureAuthenticatedThemes).toHaveBeenCalledTimes(2)
 
       expect(renderConcurrent).toHaveBeenCalledOnce()
       expect(renderConcurrent).toHaveBeenCalledWith(
@@ -166,10 +166,40 @@ describe('ThemeCommand', () => {
   })
 
   describe('multi environment', () => {
+    test('commands that act on the same store are run in groups to prevent conflicts', async () => {
+      // Given
+      vi.mocked(loadEnvironment)
+        .mockResolvedValueOnce({store: 'store1.myshopify.com', theme: 'wow a theme'})
+        .mockResolvedValueOnce({store: 'store1.myshopify.com', development: true})
+        .mockResolvedValueOnce({store: 'store2.myshopify.com', theme: 'another theme'})
+
+      vi.mocked(renderConfirmationPrompt).mockResolvedValue(true)
+      vi.mocked(renderConcurrent).mockResolvedValue(undefined)
+
+      await CommandConfig.load()
+      const command = new TestThemeCommandWithUnionFlags(
+        ['--environment', 'store1-theme', '--environment', 'store1-development', '--environment', 'store2-theme'],
+        CommandConfig,
+      )
+
+      // When
+      await command.run()
+
+      // Then
+      const runGroupOneProcesses = vi.mocked(renderConcurrent).mock.calls[0]?.[0]?.processes
+      expect(runGroupOneProcesses).toHaveLength(2)
+      expect(runGroupOneProcesses?.map((process) => process.prefix)).toEqual(['store1-theme', 'store2-theme'])
+
+      const runGroupTwoProcesses = vi.mocked(renderConcurrent).mock.calls[1]?.[0]?.processes
+      expect(runGroupTwoProcesses).toHaveLength(1)
+      expect(runGroupTwoProcesses?.map((process) => process.prefix)).toEqual(['store1-development'])
+    })
+
     test('commands with --force flag should not prompt for confirmation', async () => {
       // Given
-      const environmentConfig = {store: 'store.myshopify.com'}
-      vi.mocked(loadEnvironment).mockResolvedValue(environmentConfig)
+      vi.mocked(loadEnvironment)
+        .mockResolvedValueOnce({store: 'store1.myshopify.com', development: true})
+        .mockResolvedValueOnce({store: 'store2.myshopify.com', theme: 'staging'})
       vi.mocked(renderConfirmationPrompt).mockResolvedValue(true)
       vi.mocked(renderConcurrent).mockResolvedValue(undefined)
 
@@ -198,8 +228,9 @@ describe('ThemeCommand', () => {
 
     test('commands that do not allow --force flag should not prompt for confirmation', async () => {
       // Given
-      const environmentConfig = {store: 'store.myshopify.com'}
-      vi.mocked(loadEnvironment).mockResolvedValue(environmentConfig)
+      vi.mocked(loadEnvironment)
+        .mockResolvedValueOnce({store: 'store1.myshopify.com', development: true})
+        .mockResolvedValueOnce({store: 'store2.myshopify.com', theme: 'staging'})
       vi.mocked(renderConfirmationPrompt).mockResolvedValue(true)
       vi.mocked(renderConcurrent).mockResolvedValue(undefined)
 
@@ -216,8 +247,9 @@ describe('ThemeCommand', () => {
 
     test('commands without --force flag that allow it should prompt for confirmation', async () => {
       // Given
-      const environmentConfig = {store: 'store.myshopify.com'}
-      vi.mocked(loadEnvironment).mockResolvedValue(environmentConfig)
+      vi.mocked(loadEnvironment)
+        .mockResolvedValueOnce({store: 'store1.myshopify.com', development: true})
+        .mockResolvedValueOnce({store: 'store2.myshopify.com', theme: 'staging'})
       vi.mocked(renderConfirmationPrompt).mockResolvedValue(true)
       vi.mocked(renderConcurrent).mockResolvedValue(undefined)
 
@@ -244,8 +276,9 @@ describe('ThemeCommand', () => {
 
     test('should not execute command if confirmation is cancelled', async () => {
       // Given
-      const environmentConfig = {store: 'store.myshopify.com'}
-      vi.mocked(loadEnvironment).mockResolvedValue(environmentConfig)
+      vi.mocked(loadEnvironment)
+        .mockResolvedValueOnce({store: 'store1.myshopify.com', development: true})
+        .mockResolvedValueOnce({store: 'store2.myshopify.com', theme: 'staging'})
       vi.mocked(renderConfirmationPrompt).mockResolvedValue(false)
       vi.mocked(renderConcurrent).mockResolvedValue(undefined)
 
@@ -340,8 +373,10 @@ describe('ThemeCommand', () => {
 
     test('commands error gracefully and continue with other environments', async () => {
       // Given
-      vi.mocked(loadEnvironment).mockResolvedValue({store: 'store.myshopify.com'})
-
+      vi.mocked(loadEnvironment)
+        .mockResolvedValueOnce({store: 'store1.myshopify.com', development: true})
+        .mockResolvedValueOnce({store: 'store2.myshopify.com', theme: 'staging'})
+        .mockResolvedValueOnce({store: 'store3.myshopify.com', live: true})
       vi.mocked(renderConfirmationPrompt).mockResolvedValue(true)
       vi.mocked(renderConcurrent).mockImplementation(async ({processes}) => {
         for (const process of processes) {

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

@@ -225,33 +225,58 @@ export default abstract class ThemeCommand extends Command {
   private async runConcurrent(validEnvironments: {environment: EnvironmentName; flags: FlagValues}[]) {
     const abortController = new AbortController()
 
-    await renderConcurrent({
-      processes: validEnvironments.map(({environment, flags}) => ({
-        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 commandName = this.constructor.name.toLowerCase()
-              recordEvent(`theme-command:${commandName}:multi-env:authenticated`)
-
-              await this.command(flags, session, true, {stdout, stderr})
-            })
-
-            // eslint-disable-next-line no-catch-all/no-catch-all
-          } catch (error) {
-            if (error instanceof Error) {
-              error.message = `Environment ${environment} failed: \n\n${error.message}`
-              renderError({body: [error.message]})
+    const stores = validEnvironments.map((env) => env.flags.store as string)
+    const uniqueStores = new Set(stores)
+    const runGroups =
+      stores.length === uniqueStores.size ? [validEnvironments] : this.createSequentialGroups(validEnvironments)
+
+    for (const runGroup of runGroups) {
+      // eslint-disable-next-line no-await-in-loop
+      await renderConcurrent({
+        processes: runGroup.map(({environment, flags}) => ({
+          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 commandName = this.constructor.name.toLowerCase()
+                recordEvent(`theme-command:${commandName}:multi-env:authenticated`)
+
+                await this.command(flags, session, true, {stdout, stderr})
+              })
+
+              // eslint-disable-next-line no-catch-all/no-catch-all
+            } catch (error) {
+              if (error instanceof Error) {
+                error.message = `Environment ${environment} failed: \n\n${error.message}`
+                renderError({body: [error.message]})
+              }
             }
-          }
-        },
-      })),
-      abortSignal: abortController.signal,
-      showTimestamps: true,
+          },
+        })),
+        abortSignal: abortController.signal,
+        showTimestamps: true,
+      })
+    }
+  }
+
+  /**
+   * Create groups of environments with unique flags.store values to run sequentially
+   * to prevent conflicts between environments acting on the same store
+   * @param environments - The environments to group
+   * @returns The environment groups
+   */
+  private createSequentialGroups(environments: {environment: EnvironmentName; flags: FlagValues}[]) {
+    const groups: {environment: EnvironmentName; flags: FlagValues}[][] = []
+
+    environments.forEach((environment) => {
+      const groupWithoutStore = groups.find((arr) => !arr.some((env) => env.flags.store === environment.flags.store))
+      groupWithoutStore ? groupWithoutStore.push(environment) : groups.push([environment])
     })
+
+    return groups
   }
 
   /**