Skip to content

Commit

Permalink
Merge pull request #5323 from Shopify/01-30-refactor_devstatusmanager…
Browse files Browse the repository at this point in the history
…_from_singleton_to_injected_instance

Refactor devStatusManager from singleton to injected instance
  • Loading branch information
isaacroldan authored Jan 30, 2025
2 parents e4e7e0f + 80c1946 commit 589631d
Show file tree
Hide file tree
Showing 8 changed files with 67 additions and 24 deletions.
16 changes: 10 additions & 6 deletions packages/app/src/cli/services/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {getCachedAppInfo, setCachedAppInfo} from './local-storage.js'
import {canEnablePreviewMode} from './extensions/common.js'
import {fetchAppRemoteConfiguration} from './app/select-app.js'
import {patchAppConfigurationFile} from './app/patch-app-configuration-file.js'
import {DevSessionStatusManager} from './dev/processes/dev-session-status-manager.js'
import {DeveloperPlatformClient} from '../utilities/developer-platform-client.js'
import {Web, isCurrentAppSchema, getAppScopesArray, AppLinkedInterface} from '../models/app/app.js'
import {Organization, OrganizationApp, OrganizationStore} from '../models/organization.js'
Expand Down Expand Up @@ -71,9 +72,9 @@ export interface DevOptions {
export async function dev(commandOptions: DevOptions) {
const config = await prepareForDev(commandOptions)
await actionsBeforeSettingUpDevProcesses(config)
const {processes, graphiqlUrl, previewUrl} = await setupDevProcesses(config)
const {processes, graphiqlUrl, previewUrl, devSessionStatusManager} = await setupDevProcesses(config)
await actionsBeforeLaunchingDevProcesses(config)
await launchDevProcesses({processes, previewUrl, graphiqlUrl, config})
await launchDevProcesses({processes, previewUrl, graphiqlUrl, config, devSessionStatusManager})
}

async function prepareForDev(commandOptions: DevOptions): Promise<DevConfig> {
Expand Down Expand Up @@ -117,10 +118,10 @@ async function prepareForDev(commandOptions: DevOptions): Promise<DevConfig> {
await installAppDependencies(app)
}

const graphiqlPort = commandOptions.graphiqlPort || (await getAvailableTCPPort(ports.graphiql))
const graphiqlPort = commandOptions.graphiqlPort ?? (await getAvailableTCPPort(ports.graphiql))
const {graphiqlKey} = commandOptions

if (graphiqlPort !== (commandOptions.graphiqlPort || ports.graphiql)) {
if (graphiqlPort !== (commandOptions.graphiqlPort ?? ports.graphiql)) {
renderWarning({
headline: [
'A random port will be used for GraphiQL because',
Expand Down Expand Up @@ -224,7 +225,7 @@ export async function warnIfScopesDifferBeforeDev({
scopesMessage(getAppScopesArray(localApp.configuration)),
'\n',
'Scopes in Partner Dashboard:',
scopesMessage(remoteAccess?.scopes?.split(',') || []),
scopesMessage(remoteAccess?.scopes?.split(',') ?? []),
],
nextSteps,
})
Expand Down Expand Up @@ -301,7 +302,7 @@ async function setupNetworkingOptions(
...frontEndOptions,
tunnelClient,
}),
getBackendPort() || backendConfig?.configuration.port || getAvailableTCPPort(),
getBackendPort() ?? backendConfig?.configuration.port ?? getAvailableTCPPort(),
getURLs(remoteAppConfig),
])
const proxyUrl = usingLocalhost ? `${frontendUrl}:${proxyPort}` : frontendUrl
Expand Down Expand Up @@ -330,11 +331,13 @@ async function launchDevProcesses({
previewUrl,
graphiqlUrl,
config,
devSessionStatusManager,
}: {
processes: DevProcesses
previewUrl: string
graphiqlUrl: string | undefined
config: DevConfig
devSessionStatusManager: DevSessionStatusManager
}) {
const abortController = new AbortController()
const processesForTaskRunner: OutputProcess[] = processes.map((process) => {
Expand Down Expand Up @@ -375,6 +378,7 @@ async function launchDevProcesses({
abortController,
developerPreview: developerPreviewController(apiKey, developerPlatformClient),
shopFqdn: config.storeFqdn,
devSessionStatusManager,
})
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import {devSessionStatusManager, DevSessionStatus} from './dev-session-status-manager.js'
import {DevSessionStatus, DevSessionStatusManager} from './dev-session-status-manager.js'
import {describe, test, expect, beforeEach, vi} from 'vitest'

describe('DevSessionStatusManager', () => {
const devSessionStatusManager = new DevSessionStatusManager()

beforeEach(() => {
devSessionStatusManager.removeAllListeners()
devSessionStatusManager.reset()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ export class DevSessionStatusManager extends EventEmitter {
graphiqlURL: undefined,
}

constructor(defaultStatus?: DevSessionStatus) {
super()
if (defaultStatus) this.currentStatus = defaultStatus
}

updateStatus(status: Partial<DevSessionStatus>) {
const newStatus = {...this.currentStatus, ...status}
// Only emit if status has changed
Expand All @@ -35,5 +40,3 @@ export class DevSessionStatusManager extends EventEmitter {
}
}
}

export const devSessionStatusManager = new DevSessionStatusManager()
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {setupDevSessionProcess, pushUpdatesForDevSession} from './dev-session.js'
import {devSessionStatusManager} from './dev-session-status-manager.js'
import {DevSessionStatusManager} from './dev-session-status-manager.js'
import {DeveloperPlatformClient} from '../../../utilities/developer-platform-client.js'
import {AppLinkedInterface} from '../../../models/app/app.js'
import {AppEventWatcher} from '../app-events/app-event-watcher.js'
Expand Down Expand Up @@ -39,6 +39,7 @@ describe('setupDevSessionProcess', () => {
appWatcher: {} as AppEventWatcher,
appPreviewURL: 'https://test.preview.url',
appLocalProxyURL: 'https://test.local.url',
devSessionStatusManager: new DevSessionStatusManager(),
}

// When
Expand All @@ -60,6 +61,7 @@ describe('setupDevSessionProcess', () => {
appWatcher: options.appWatcher,
appPreviewURL: options.appPreviewURL,
appLocalProxyURL: options.appLocalProxyURL,
devSessionStatusManager: options.devSessionStatusManager,
},
})
})
Expand All @@ -73,6 +75,7 @@ describe('pushUpdatesForDevSession', () => {
let appWatcher: AppEventWatcher
let app: AppLinkedInterface
let abortController: AbortController
let devSessionStatusManager: DevSessionStatusManager

beforeEach(() => {
vi.mocked(formData).mockReturnValue({append: vi.fn(), getHeaders: vi.fn()} as any)
Expand All @@ -83,7 +86,7 @@ describe('pushUpdatesForDevSession', () => {
app = testAppLinked()
appWatcher = new AppEventWatcher(app)
abortController = new AbortController()
devSessionStatusManager.reset()
devSessionStatusManager = new DevSessionStatusManager()
options = {
developerPlatformClient,
appWatcher,
Expand All @@ -92,6 +95,7 @@ describe('pushUpdatesForDevSession', () => {
organizationId: 'org123',
appPreviewURL: 'https://test.preview.url',
appLocalProxyURL: 'https://test.local.url',
devSessionStatusManager,
}
})

Expand Down
13 changes: 7 additions & 6 deletions packages/app/src/cli/services/dev/processes/dev-session.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {BaseProcess, DevProcessFunction} from './types.js'
import {devSessionStatusManager} from './dev-session-status-manager.js'
import {DevSessionStatusManager} from './dev-session-status-manager.js'
import {DeveloperPlatformClient} from '../../../utilities/developer-platform-client.js'
import {AppLinkedInterface} from '../../../models/app/app.js'
import {getExtensionUploadURL} from '../../deploy/upload.js'
Expand Down Expand Up @@ -31,6 +31,7 @@ interface DevSessionOptions {
appWatcher: AppEventWatcher
appPreviewURL: string
appLocalProxyURL: string
devSessionStatusManager: DevSessionStatusManager
}

interface DevSessionProcessOptions extends DevSessionOptions {
Expand Down Expand Up @@ -88,7 +89,7 @@ export const pushUpdatesForDevSession: DevProcessFunction<DevSessionOptions> = a
{stderr, stdout, abortSignal: signal},
options,
) => {
const {appWatcher} = options
const {appWatcher, devSessionStatusManager} = options

const processOptions = {...options, stderr, stdout, signal, bundlePath: appWatcher.buildOutputPath}

Expand Down Expand Up @@ -164,7 +165,7 @@ async function handleDevSessionResult(
await printSuccess(`✅ Updated`, processOptions.stdout)
await printActionRequiredMessages(processOptions, event)
} else if (result.status === 'created') {
devSessionStatusManager.updateStatus({isReady: true})
processOptions.devSessionStatusManager.updateStatus({isReady: true})
await printSuccess(`✅ Ready, watching for changes in your app `, processOptions.stdout)
} else if (result.status === 'aborted') {
outputDebug('❌ Session update aborted (new change detected or error in Session Update)', processOptions.stdout)
Expand All @@ -174,7 +175,7 @@ async function handleDevSessionResult(

// If we failed to create a session, exit the process. Don't throw an error in tests as it can't be caught due to the
// async nature of the process.
if (!devSessionStatusManager.status.isReady && !isUnitTest()) {
if (!processOptions.devSessionStatusManager.status.isReady && !isUnitTest()) {
throw new AbortError('Failed to start dev session.')
}
}
Expand Down Expand Up @@ -253,7 +254,7 @@ async function bundleExtensionsAndUpload(options: DevSessionProcessOptions): Pro
// Create or update the dev session
if (currentBundleController.signal.aborted) return {status: 'aborted'}
try {
if (devSessionStatusManager.status.isReady) {
if (options.devSessionStatusManager.status.isReady) {
const result = await devSessionUpdateWithRetry(payload, options.developerPlatformClient)
const errors = result.devSessionUpdate?.userErrors ?? []
if (errors.length) return {status: 'remote-error', error: errors}
Expand Down Expand Up @@ -363,5 +364,5 @@ async function printLogMessage(message: string, stdout: Writable, prefix?: strin
async function updatePreviewURL(options: DevSessionProcessOptions, event: AppEvent) {
const hasPreview = event.app.allExtensions.filter((ext) => ext.isPreviewable).length > 0
const newPreviewURL = hasPreview ? options.appLocalProxyURL : options.appPreviewURL
devSessionStatusManager.updateStatus({previewURL: newPreviewURL})
options.devSessionStatusManager.updateStatus({previewURL: newPreviewURL})
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {WebProcess, setupWebProcesses} from './web.js'
import {DevSessionProcess, setupDevSessionProcess} from './dev-session.js'
import {AppLogsSubscribeProcess, setupAppLogsPollingProcess} from './app-logs-polling.js'
import {AppWatcherProcess, setupAppWatcherProcess} from './app-watcher-process.js'
import {devSessionStatusManager} from './dev-session-status-manager.js'
import {DevSessionStatusManager} from './dev-session-status-manager.js'
import {environmentVariableNames} from '../../../constants.js'
import {AppLinkedInterface, getAppScopes, WebType} from '../../../models/app/app.js'

Expand Down Expand Up @@ -79,6 +79,7 @@ export async function setupDevProcesses({
processes: DevProcesses
previewUrl: string
graphiqlUrl: string | undefined
devSessionStatusManager: DevSessionStatusManager
}> {
const apiKey = remoteApp.apiKey
const apiSecret = remoteApp.apiSecretKeys[0]?.secret ?? ''
Expand All @@ -100,7 +101,7 @@ export async function setupDevProcesses({
? `http://localhost:${graphiqlPort}/graphiql${graphiqlKey ? `?key=${graphiqlKey}` : ''}`
: undefined

devSessionStatusManager.updateStatus({isReady: false, previewURL, graphiqlURL})
const devSessionStatusManager = new DevSessionStatusManager({isReady: false, previewURL, graphiqlURL})

const processes = [
...(await setupWebProcesses({
Expand Down Expand Up @@ -150,6 +151,7 @@ export async function setupDevProcesses({
appWatcher,
appPreviewURL: appPreviewUrl,
appLocalProxyURL: devConsoleURL,
devSessionStatusManager,
})
: await setupDraftableExtensionsProcess({
localApp: reloadedApp,
Expand Down Expand Up @@ -198,6 +200,7 @@ export async function setupDevProcesses({
processes: processesWithProxy,
previewUrl: previewURL,
graphiqlUrl: graphiqlURL,
devSessionStatusManager,
}
}

Expand Down
31 changes: 28 additions & 3 deletions packages/app/src/cli/services/dev/ui.test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {renderDev} from './ui.js'
import {Dev} from './ui/components/Dev.js'
import {DevSessionUI} from './ui/components/DevSessionUI.js'
import {devSessionStatusManager} from './processes/dev-session-status-manager.js'
import {DevSessionStatusManager} from './processes/dev-session-status-manager.js'
import {testDeveloperPlatformClient} from '../../models/app/app.test-data.js'
import {afterEach, describe, expect, test, vi} from 'vitest'
import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output'
Expand All @@ -21,6 +21,7 @@ const developerPreview = {
}

const developerPlatformClient = testDeveloperPlatformClient()
const devSessionStatusManager = new DevSessionStatusManager()

afterEach(() => {
mockAndCaptureOutput().clear()
Expand Down Expand Up @@ -60,6 +61,7 @@ describe('ui', () => {
abortController,
developerPreview,
shopFqdn,
devSessionStatusManager,
})

expect(vi.mocked(Dev)).not.toHaveBeenCalled()
Expand Down Expand Up @@ -103,6 +105,7 @@ describe('ui', () => {
abortController,
developerPreview,
shopFqdn,
devSessionStatusManager,
})
abortController.abort()

Expand Down Expand Up @@ -142,6 +145,7 @@ describe('ui', () => {
abortController,
developerPreview,
shopFqdn,
devSessionStatusManager,
})
abortController.abort()

Expand Down Expand Up @@ -173,7 +177,17 @@ describe('ui', () => {
const abortController = new AbortController()

// eslint-disable-next-line @typescript-eslint/no-floating-promises
renderDev({processes, previewUrl, graphiqlUrl, graphiqlPort, app, abortController, developerPreview, shopFqdn})
renderDev({
processes,
previewUrl,
graphiqlUrl,
graphiqlPort,
app,
abortController,
developerPreview,
shopFqdn,
devSessionStatusManager,
})

await new Promise((resolve) => setTimeout(resolve, 100))

Expand Down Expand Up @@ -209,7 +223,17 @@ describe('ui', () => {
const abortController = new AbortController()

// eslint-disable-next-line @typescript-eslint/no-floating-promises
renderDev({processes, previewUrl, graphiqlUrl, graphiqlPort, app, abortController, developerPreview, shopFqdn})
renderDev({
processes,
previewUrl,
graphiqlUrl,
graphiqlPort,
app,
abortController,
developerPreview,
shopFqdn,
devSessionStatusManager,
})

await new Promise((resolve) => setTimeout(resolve, 100))

Expand Down Expand Up @@ -258,6 +282,7 @@ describe('ui', () => {
abortController,
developerPreview,
shopFqdn,
devSessionStatusManager,
})

await new Promise((resolve) => setTimeout(resolve, 100))
Expand Down
5 changes: 3 additions & 2 deletions packages/app/src/cli/services/dev/ui.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {Dev, DevProps} from './ui/components/Dev.js'
import {DevSessionUI} from './ui/components/DevSessionUI.js'
import {devSessionStatusManager} from './processes/dev-session-status-manager.js'
import {DevSessionStatusManager} from './processes/dev-session-status-manager.js'
import React from 'react'
import {render} from '@shopify/cli-kit/node/ui'
import {terminalSupportsPrompting} from '@shopify/cli-kit/node/system'
Expand All @@ -16,7 +16,8 @@ export async function renderDev({
graphiqlPort,
developerPreview,
shopFqdn,
}: DevProps) {
devSessionStatusManager,
}: DevProps & {devSessionStatusManager: DevSessionStatusManager}) {
if (!terminalSupportsPrompting()) {
await renderDevNonInteractive({processes, app, abortController, developerPreview, shopFqdn})
} else if (app.developerPlatformClient.supportsDevSessions) {
Expand Down

0 comments on commit 589631d

Please sign in to comment.