Skip to content

Commit cdc4f71

Browse files
EvilGenius13EvilGenius13
authored
Merge pull request #5954 from Shopify/fix-single-environment-theme-calls
Update behaviour for theme command calls
2 parents 48e3eb4 + ef64211 commit cdc4f71

File tree

3 files changed

+193
-28
lines changed

3 files changed

+193
-28
lines changed

‎.changeset/itchy-houses-kick.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
'@shopify/theme': patch
3+
---
4+
5+
Improvement and fixes when handling multi-environment commands
6+
7+
- Fixes a bug where passing a single environment to multi-env commands would cause it to fail if the environment didn't have all of the required attributes for multi-env
8+
- Updates output when running multi-env commands to ensure the results from each command don't overlap one another

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

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
import ThemeCommand from './theme-command.js'
2+
import {describe, vi, expect, test, beforeEach} from 'vitest'
3+
import {Config, Flags} from '@oclif/core'
4+
import {AdminSession, ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
5+
import {loadEnvironment} from '@shopify/cli-kit/node/environments'
6+
import {renderConcurrent} from '@shopify/cli-kit/node/ui'
7+
import type {Writable} from 'stream'
8+
9+
vi.mock('@shopify/cli-kit/node/session')
10+
vi.mock('@shopify/cli-kit/node/environments')
11+
vi.mock('@shopify/cli-kit/node/ui')
12+
13+
const CommandConfig = new Config({root: __dirname})
14+
15+
class TestThemeCommand extends ThemeCommand {
16+
static flags = {
17+
environment: Flags.string({
18+
multiple: true,
19+
default: [],
20+
env: 'SHOPIFY_FLAG_ENVIRONMENT',
21+
}),
22+
store: Flags.string({
23+
env: 'SHOPIFY_FLAG_STORE',
24+
}),
25+
password: Flags.string({
26+
env: 'SHOPIFY_FLAG_PASSWORD',
27+
}),
28+
}
29+
30+
static multiEnvironmentsFlags = ['store']
31+
32+
commandCalls: {flags: any; session: AdminSession; context?: any}[] = []
33+
34+
async command(flags: any, session: AdminSession, context?: {stdout?: Writable; stderr?: Writable}): Promise<void> {
35+
this.commandCalls.push({flags, session, context})
36+
}
37+
}
38+
39+
describe('ThemeCommand', () => {
40+
let mockSession: AdminSession
41+
42+
beforeEach(() => {
43+
mockSession = {
44+
token: 'test-token',
45+
storeFqdn: 'test-store.myshopify.com',
46+
}
47+
vi.mocked(ensureAuthenticatedThemes).mockResolvedValue(mockSession)
48+
})
49+
50+
describe('run', () => {
51+
test('no environment provided', async () => {
52+
// Given
53+
await CommandConfig.load()
54+
const command = new TestThemeCommand([], CommandConfig)
55+
56+
// When
57+
await command.run()
58+
59+
// Then
60+
expect(ensureAuthenticatedThemes).toHaveBeenCalledOnce()
61+
expect(loadEnvironment).not.toHaveBeenCalled()
62+
expect(renderConcurrent).not.toHaveBeenCalled()
63+
expect(command.commandCalls).toHaveLength(1)
64+
expect(command.commandCalls[0]).toMatchObject({
65+
flags: {environment: []},
66+
session: mockSession,
67+
context: undefined,
68+
})
69+
})
70+
71+
test('single environment provided', async () => {
72+
// Given
73+
const environmentConfig = {store: 'env-store.myshopify.com'}
74+
vi.mocked(loadEnvironment).mockResolvedValue(environmentConfig)
75+
76+
await CommandConfig.load()
77+
const command = new TestThemeCommand(['--environment', 'development'], CommandConfig)
78+
79+
// When
80+
await command.run()
81+
82+
// Then
83+
expect(loadEnvironment).toHaveBeenCalledWith('development', 'shopify.theme.toml', {from: undefined})
84+
expect(ensureAuthenticatedThemes).toHaveBeenCalledTimes(1)
85+
expect(renderConcurrent).not.toHaveBeenCalled()
86+
expect(command.commandCalls).toHaveLength(1)
87+
expect(command.commandCalls[0]).toMatchObject({
88+
flags: {
89+
environment: ['development'],
90+
store: 'env-store.myshopify.com',
91+
},
92+
session: mockSession,
93+
context: undefined,
94+
})
95+
})
96+
97+
test('multiple environments provided - uses renderConcurrent for parallel execution', async () => {
98+
// Given
99+
const environmentConfig = {store: 'store.myshopify.com'}
100+
vi.mocked(loadEnvironment).mockResolvedValue(environmentConfig)
101+
vi.mocked(ensureAuthenticatedThemes).mockResolvedValue(mockSession)
102+
103+
vi.mocked(renderConcurrent).mockResolvedValue(undefined)
104+
105+
await CommandConfig.load()
106+
const command = new TestThemeCommand(['--environment', 'development', '--environment', 'staging'], CommandConfig)
107+
108+
// When
109+
await command.run()
110+
111+
// Then
112+
expect(loadEnvironment).toHaveBeenCalledWith('development', 'shopify.theme.toml', {from: undefined, silent: true})
113+
expect(loadEnvironment).toHaveBeenCalledWith('staging', 'shopify.theme.toml', {from: undefined, silent: true})
114+
expect(ensureAuthenticatedThemes).toHaveBeenCalledTimes(2)
115+
116+
expect(renderConcurrent).toHaveBeenCalledOnce()
117+
expect(renderConcurrent).toHaveBeenCalledWith(
118+
expect.objectContaining({
119+
processes: expect.arrayContaining([
120+
expect.objectContaining({prefix: 'development'}),
121+
expect.objectContaining({prefix: 'staging'}),
122+
]),
123+
showTimestamps: true,
124+
}),
125+
)
126+
})
127+
})
128+
})

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

Lines changed: 57 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@ import {ArgOutput, FlagOutput, Input} from '@oclif/core/lib/interfaces/parser.js
44
import Command from '@shopify/cli-kit/node/base-command'
55
import {AdminSession, ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
66
import {loadEnvironment} from '@shopify/cli-kit/node/environments'
7-
import {renderWarning} from '@shopify/cli-kit/node/ui'
7+
import {renderWarning, renderConcurrent} from '@shopify/cli-kit/node/ui'
88
import {AbortError} from '@shopify/cli-kit/node/error'
9+
import {AbortController} from '@shopify/cli-kit/node/abort'
10+
import type {Writable} from 'stream'
911

1012
export interface FlagValues {
1113
[key: string]: boolean | string | string[] | number | undefined
@@ -36,7 +38,11 @@ export default abstract class ThemeCommand extends Command {
3638
return configurationFileName
3739
}
3840

39-
async command(_flags: FlagValues, _session: AdminSession): Promise<void> {}
41+
async command(
42+
_flags: FlagValues,
43+
_session: AdminSession,
44+
_context?: {stdout?: Writable; stderr?: Writable},
45+
): Promise<void> {}
4046

4147
async run<
4248
TFlags extends FlagOutput & {path?: string; verbose?: boolean},
@@ -50,50 +56,73 @@ export default abstract class ThemeCommand extends Command {
5056
const requiredFlags = klass.multiEnvironmentsFlags
5157
const {flags} = await this.parse(klass)
5258

53-
// Single environment
54-
if (!flags.environment) {
59+
// No environment provided
60+
if (!flags.environment?.length) {
5561
const session = await this.ensureAuthenticated(flags)
62+
5663
await this.command(flags, session)
64+
5765
return
5866
}
5967

60-
// Synchronously authenticate all environments
61-
const sessions: {[storeFqdn: string]: AdminSession} = {}
6268
// OCLIF parses flags.environment as an array when using the --environment & -e flag but
6369
// as a string when using the direct environment variable SHOPIFY_FLAG_ENVIRONMENT
6470
// This handles both cases
6571
const environments = Array.isArray(flags.environment) ? flags.environment : [flags.environment]
6672

73+
// If only one environment is specified, treat it as single environment mode
74+
if (environments.length === 1) {
75+
const session = await this.ensureAuthenticated(flags)
76+
await this.command(flags, session)
77+
return
78+
}
79+
80+
// Multiple environments
81+
const sessions: {[storeFqdn: string]: AdminSession} = {}
82+
6783
// Authenticate on all environments sequentially to avoid race conditions,
6884
// with authentication happening in parallel.
6985
for (const environmentName of environments) {
7086
// eslint-disable-next-line no-await-in-loop
71-
const environmentConfig = await loadEnvironment(environmentName, 'shopify.theme.toml', {from: flags.path})
87+
const environmentConfig = await loadEnvironment(environmentName, 'shopify.theme.toml', {
88+
from: flags.path,
89+
silent: true,
90+
})
7291
// eslint-disable-next-line no-await-in-loop
7392
sessions[environmentName] = await this.ensureAuthenticated(environmentConfig as FlagValues)
7493
}
7594

76-
// Concurrently run commands
77-
await Promise.all(
78-
environments.map(async (environment: string) => {
79-
const environmentConfig = await loadEnvironment(environment, 'shopify.theme.toml', {from: flags.path})
80-
const environmentFlags = {
81-
...flags,
82-
...environmentConfig,
83-
environment: [environment],
84-
}
85-
86-
if (!this.validConfig(environmentConfig as FlagValues, requiredFlags, environment)) return
87-
88-
const session = sessions[environment]
89-
90-
if (!session) {
91-
throw new AbortError(`No session found for environment ${environment}`)
92-
}
93-
94-
return this.command(environmentFlags, session)
95-
}),
96-
)
95+
// Use renderConcurrent for multi-environment execution
96+
const abortController = new AbortController()
97+
98+
await renderConcurrent({
99+
processes: environments.map((environment: string) => ({
100+
prefix: environment,
101+
action: async (stdout: Writable, stderr: Writable, _signal) => {
102+
const environmentConfig = await loadEnvironment(environment, 'shopify.theme.toml', {
103+
from: flags.path,
104+
silent: true,
105+
})
106+
const environmentFlags = {
107+
...flags,
108+
...environmentConfig,
109+
environment: [environment],
110+
}
111+
112+
if (!this.validConfig(environmentConfig as FlagValues, requiredFlags, environment)) return
113+
114+
const session = sessions[environment]
115+
116+
if (!session) {
117+
throw new AbortError(`No session found for environment ${environment}`)
118+
}
119+
120+
await this.command(environmentFlags, session, {stdout, stderr})
121+
},
122+
})),
123+
abortSignal: abortController.signal,
124+
showTimestamps: true,
125+
})
97126
}
98127

99128
private async ensureAuthenticated(flags: FlagValues) {

0 commit comments

Comments
 (0)