Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Themes] Remove Ruby invocation from the app dev command #4592

Merged
merged 10 commits into from
Oct 16, 2024
6 changes: 0 additions & 6 deletions packages/app/src/cli/commands/app/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,6 @@ If you're using the PHP or Ruby app template, then you need to complete the foll
'Key used to authenticate GraphiQL requests. Should be specified if exposing GraphiQL on a publicly accessible URL. By default, no key is required.',
env: 'SHOPIFY_FLAG_GRAPHIQL_KEY',
}),
legacy: Flags.boolean({
hidden: true,
description: 'Use the legacy Ruby implementation for managing theme app extensions.',
env: 'SHOPIFY_FLAG_LEGACY',
}),
}

public static analyticsStopCommand(): string | undefined {
Expand Down Expand Up @@ -185,7 +180,6 @@ If you're using the PHP or Ruby app template, then you need to complete the foll
notify: flags.notify,
graphiqlPort: flags['graphiql-port'],
graphiqlKey: flags['graphiql-key'],
devPreview: !flags.legacy,
}

const result = await dev(devOptions)
Expand Down
1 change: 0 additions & 1 deletion packages/app/src/cli/services/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ export interface DevOptions {
notify?: string
graphiqlPort?: number
graphiqlKey?: string
devPreview?: boolean
}

export async function dev(commandOptions: DevOptions) {
Expand Down
64 changes: 35 additions & 29 deletions packages/app/src/cli/services/dev/extension/bundler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {FSWatcher} from 'chokidar'
import micromatch from 'micromatch'
import {deepCompare} from '@shopify/cli-kit/common/object'
import {Writable} from 'stream'
import {AsyncResource} from 'async_hooks'

export interface FileWatcherOptions {
devOptions: ExtensionDevOptions
Expand Down Expand Up @@ -170,36 +171,41 @@ Redeploy Paths:

let buildController: AbortController | null
const allPaths = [...buildPaths, ...configurationPaths]
const functionRebuildAndRedeployWatcher = chokidar.watch(allPaths, {ignored: '**/*.test.*'}).on('change', (path) => {
outputDebug(`Extension file at path ${path} changed`, stdout)
if (buildController) {
// terminate any existing builds
buildController.abort()
}
buildController = new AbortController()
const buildSignal = buildController.signal
const shouldBuild = micromatch.isMatch(path, buildPaths)

reloadAndbuildIfNecessary(extension, shouldBuild, {
app,
stdout,
stderr,
useTasks: false,
signal: buildSignal,
environment: 'development',
appURL: url,
})
.then(({newConfig, previousConfig}) => {
if (shouldBuild) {
if (buildSignal.aborted) return
return onChange()
}

if (deepCompare(newConfig, previousConfig)) return
return onChange()
const functionRebuildAndRedeployWatcher = chokidar.watch(allPaths, {ignored: '**/*.test.*'}).on(
'change',
// We need to bind the execution context to ensure the event handler can access the correct AsyncLocalStorage
// See also: https://nodejs.org/api/async_context.html#integrating-asyncresource-with-eventemitter
AsyncResource.bind((path) => {
outputDebug(`Extension file at path ${path} changed`, stdout)
if (buildController) {
// terminate any existing builds
buildController.abort()
}
buildController = new AbortController()
const buildSignal = buildController.signal
const shouldBuild = micromatch.isMatch(path, buildPaths)

reloadAndbuildIfNecessary(extension, shouldBuild, {
app,
stdout,
stderr,
useTasks: false,
signal: buildSignal,
environment: 'development',
appURL: url,
})
.catch((error: Error) => onReloadAndBuildError(error))
})
.then(({newConfig, previousConfig}) => {
if (shouldBuild) {
if (buildSignal.aborted) return
return onChange()
}

if (deepCompare(newConfig, previousConfig)) return
return onChange()
})
.catch((error: Error) => onReloadAndBuildError(error))
}),
)
listenForAbortOnWatcher(functionRebuildAndRedeployWatcher)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import {WebProcess, launchWebProcess} from './web.js'
import {PreviewableExtensionProcess, launchPreviewableExtensionProcess} from './previewable-extension.js'
import {launchGraphiQLServer} from './graphiql.js'
import {pushUpdatesForDraftableExtensions} from './draftable-extension.js'
import {runThemeAppExtensionsServer} from './theme-app-extension.js'
import {pushUpdatesForDevSession} from './dev-session.js'
import {runThemeAppExtensionsServer} from './theme-app-extension.js'
import {
testAppAccessConfigExtension,
testAppConfigExtensions,
Expand All @@ -26,12 +26,15 @@ import {describe, test, expect, beforeEach, vi} from 'vitest'
import {ensureAuthenticatedAdmin, ensureAuthenticatedStorefront} from '@shopify/cli-kit/node/session'
import {Config} from '@oclif/core'
import {getEnvironmentVariables} from '@shopify/cli-kit/node/environment'
import {isStorefrontPasswordProtected} from '@shopify/theme'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated these tests to assert that the process list that is built uses the TS implementation coming from theme-app-extension-next rather than the legacy implementation

import {fetchTheme} from '@shopify/cli-kit/node/themes/api'

vi.mock('../../context/identifiers.js')
vi.mock('@shopify/cli-kit/node/session.js')
vi.mock('../fetch.js')
vi.mock('@shopify/cli-kit/node/environment')

vi.mock('@shopify/theme')
vi.mock('@shopify/cli-kit/node/themes/api')
beforeEach(() => {
// mocked for draft extensions
vi.mocked(ensureDeploymentIdsPresence).mockResolvedValue({
Expand All @@ -48,6 +51,14 @@ beforeEach(() => {
})
vi.mocked(ensureAuthenticatedStorefront).mockResolvedValue('storefront-token')
vi.mocked(getEnvironmentVariables).mockReturnValue({})
vi.mocked(isStorefrontPasswordProtected).mockResolvedValue(false)
vi.mocked(fetchTheme).mockResolvedValue({
id: 1,
name: 'Theme',
createdAtRuntime: false,
role: 'theme',
processing: false,
})
})

describe('setup-dev-processes', () => {
Expand Down Expand Up @@ -199,16 +210,19 @@ describe('setup-dev-processes', () => {
prefix: 'theme-extensions',
function: runThemeAppExtensionsServer,
options: {
theme: {
id: 1,
name: 'Theme',
createdAtRuntime: false,
role: 'theme',
processing: false,
},
adminSession: {
storeFqdn: 'store.myshopify.io',
token: 'admin-token',
},
themeExtensionServerArgs:
'./my-extension --api-key api-key --extension-id extension-id --extension-title theme-extension-name --extension-type THEME_APP_EXTENSION --theme 1'.split(
' ',
),
storefrontToken: 'storefront-token',
developerPlatformClient,
themeExtensionDirectory: './my-extension',
themeExtensionPort: 9293,
},
})
expect(res.processes[5]).toMatchObject({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
import {BaseProcess, DevProcessFunction} from './types.js'
import {PreviewThemeAppExtensionsProcess, setupPreviewThemeAppExtensionsProcess} from './theme-app-extension.js'
import {
PreviewThemeAppExtensionsProcess as PreviewThemeAppExtensionsNextProcess,
setupPreviewThemeAppExtensionsProcess as setupPreviewThemeAppExtensionsProcessNext,
} from './theme-app-extension-next.js'
import {PreviewableExtensionProcess, setupPreviewableExtensionsProcess} from './previewable-extension.js'
import {DraftableExtensionProcess, setupDraftableExtensionsProcess} from './draftable-extension.js'
import {SendWebhookProcess, setupSendUninstallWebhookProcess} from './uninstall-webhook.js'
Expand Down Expand Up @@ -31,7 +27,6 @@ interface ProxyServerProcess extends BaseProcess<{port: number; rules: {[key: st
type DevProcessDefinition =
| SendWebhookProcess
| PreviewThemeAppExtensionsProcess
| PreviewThemeAppExtensionsNextProcess
| WebProcess
| ProxyServerProcess
| PreviewableExtensionProcess
Expand Down Expand Up @@ -141,24 +136,13 @@ export async function setupDevProcesses({
developerPlatformClient,
proxyUrl: network.proxyUrl,
}),
commandOptions.devPreview
? await setupPreviewThemeAppExtensionsProcessNext({
remoteApp,
localApp,
storeFqdn,
developerPlatformClient,
theme: commandOptions.theme,
themeExtensionPort: commandOptions.themeExtensionPort,
})
: await setupPreviewThemeAppExtensionsProcess({
allExtensions: localApp.allExtensions,
storeFqdn,
apiKey,
developerPlatformClient,
theme: commandOptions.theme,
themeExtensionPort: commandOptions.themeExtensionPort,
notify: commandOptions.notify,
}),
await setupPreviewThemeAppExtensionsProcess({
remoteApp,
localApp,
storeFqdn,
theme: commandOptions.theme,
themeExtensionPort: commandOptions.themeExtensionPort,
}),
setupSendUninstallWebhookProcess({
webs: localApp.webs,
backendPort: network.backendPort,
Expand Down

This file was deleted.

Loading
Loading