Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ vi.mock('../../services/build/extension.js', async () => {
return {
...actual,
buildUIExtension: vi.fn(),
buildThemeExtension: vi.fn(),
buildFunctionExtension: vi.fn(),
}
})
Expand Down Expand Up @@ -148,8 +147,16 @@ describe('build', async () => {
// Given
const extensionInstance = await testTaxCalculationExtension(tmpDir)
const options: ExtensionBuildOptions = {
stdout: new Writable(),
stderr: new Writable(),
stdout: new Writable({
write(chunk, enc, cb) {
cb()
},
}),
stderr: new Writable({
write(chunk, enc, cb) {
cb()
},
}),
app: testApp(),
environment: 'production',
}
Expand Down
58 changes: 24 additions & 34 deletions packages/app/src/cli/models/extensions/extension-instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,19 @@ import {PrivacyComplianceWebhooksSpecIdentifier} from './specifications/app_conf
import {WebhooksSpecIdentifier} from './specifications/app_config_webhook.js'
import {WebhookSubscriptionSpecIdentifier} from './specifications/app_config_webhook_subscription.js'
import {EventsSpecIdentifier} from './specifications/app_config_events.js'
import {
ExtensionBuildOptions,
buildFunctionExtension,
buildThemeExtension,
buildUIExtension,
bundleFunctionExtension,
} from '../../services/build/extension.js'
import {bundleThemeExtension, copyFilesForExtension} from '../../services/extensions/bundle.js'
import {ExtensionBuildOptions, bundleFunctionExtension} from '../../services/build/extension.js'
import {bundleThemeExtension} from '../../services/extensions/bundle.js'
import {Identifiers} from '../app/identifiers.js'
import {DeveloperPlatformClient} from '../../utilities/developer-platform-client.js'
import {AppConfigurationWithoutPath} from '../app/app.js'
import {ApplicationURLs} from '../../services/dev/urls.js'
import {executeStep, BuildContext} from '../../services/build/client-steps.js'
import {ok} from '@shopify/cli-kit/node/result'
import {constantize, slugify} from '@shopify/cli-kit/common/string'
import {hashString, nonRandomUUID} from '@shopify/cli-kit/node/crypto'
import {partnersFqdn} from '@shopify/cli-kit/node/context/fqdn'
import {joinPath, basename, normalizePath, resolvePath} from '@shopify/cli-kit/node/path'
import {fileExists, touchFile, moveFile, writeFile, glob, copyFile, globSync} from '@shopify/cli-kit/node/fs'
import {fileExists, moveFile, glob, copyFile, globSync} from '@shopify/cli-kit/node/fs'
import {getPathValue} from '@shopify/cli-kit/common/object'
import {outputDebug} from '@shopify/cli-kit/node/output'
import {extractJSImports, extractImportPathsRecursively} from '@shopify/cli-kit/node/import-extractor'
Expand Down Expand Up @@ -138,7 +133,7 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi

get outputFileName() {
const mode = this.specification.buildConfig.mode
if (mode === 'copy_files' || mode === 'theme') {
if (mode === 'hosted_app_home' || mode === 'theme') {
return ''
} else if (mode === 'function') {
return 'index.wasm'
Expand Down Expand Up @@ -348,31 +343,26 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi
}

async build(options: ExtensionBuildOptions): Promise<void> {
const mode = this.specification.buildConfig.mode
const {clientSteps} = this.specification

const context: BuildContext = {
extension: this,
options,
stepResults: new Map(),
}

switch (mode) {
case 'theme':
await buildThemeExtension(this, options)
return bundleThemeExtension(this, options)
case 'function':
return buildFunctionExtension(this, options)
case 'ui':
await buildUIExtension(this, options)
// Copy static assets after build completes
return this.copyStaticAssets()
case 'tax_calculation':
await touchFile(this.outputPath)
await writeFile(this.outputPath, '(()=>{})();')
break
case 'copy_files':
return copyFilesForExtension(
this,
options,
this.specification.buildConfig.filePatterns ?? [],
this.specification.buildConfig.ignoredFilePatterns ?? [],
)
case 'none':
break
const steps = clientSteps
.filter((lifecycle) => lifecycle.lifecycle === 'deploy')
.flatMap((lifecycle) => lifecycle.steps)

Choose a reason for hiding this comment

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

Crash when specification.clientSteps is missing/undefined

ExtensionInstance.build() now does:

const steps = clientSteps
  .filter((lifecycle) => lifecycle.lifecycle === 'deploy')
  .flatMap((lifecycle) => lifecycle.steps)

If this.specification.clientSteps is undefined for any extension type (including internal/legacy specs not updated in this PR), this will throw: TypeError: Cannot read properties of undefined (reading 'filter').

Impact:

  • Extension builds/deploys will hard-fail for any extension type missing clientSteps, blocking development and deployments.
  • CI/release pipelines that build extensions will fail; could break deploys for affected extension types.
  • Potentially broad depending on how many specs are migrated; PR only updates some specs so risk is high.
  • Could cause widespread deployment failures.


for (const step of steps) {
// eslint-disable-next-line no-await-in-loop
const result = await executeStep(step, context)
context.stepResults.set(step.id, result)

if (!result.success && !step.continueOnError) {
throw new Error(`Build step "${step.name}" failed: ${result.error?.message}`)
}

Choose a reason for hiding this comment

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

Duplicate/double-wrapped error handling in ExtensionInstance.build()

executeStep() already throws when a step fails and continueOnError is false:

throw new Error(`Build step "${step.displayName}" failed: ${stepError.message}`)

But ExtensionInstance.build() then checks:

if (!result.success && !step.continueOnError) {
  throw new Error(`Build step "${step.displayName}" failed: ${result.error?.message}`)
}

That condition is effectively unreachable because executeStep() won’t return {success:false} unless continueOnError is true. This is dead logic and also risks producing a worse error message (e.g., undefined) if behavior changes later.

Impact:

  • User impact: Confusing/duplicated error messaging during builds; harder debugging.
  • Infra impact: No direct outage risk, but increases operational toil.

}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
import spec from './channel.js'
import {ExtensionInstance} from '../extension-instance.js'
import {ExtensionBuildOptions} from '../../../services/build/extension.js'
import {describe, expect, test} from 'vitest'
import {inTemporaryDirectory, writeFile, fileExists, mkdir} from '@shopify/cli-kit/node/fs'
import {joinPath} from '@shopify/cli-kit/node/path'
import {Writable} from 'stream'

const SUBDIRECTORY = 'specifications'

describe('channel_config', () => {
describe('clientSteps', () => {
test('uses copy_files mode', () => {
expect(spec.buildConfig.mode).toBe('copy_files')
})

test('has a single copy-files step scoped to the specifications subdirectory', () => {
expect(spec.clientSteps[0]!.steps).toHaveLength(1)
expect(spec.clientSteps[0]!.steps[0]).toMatchObject({
id: 'copy-files',
type: 'include_assets',
config: {
inclusions: [{type: 'pattern', baseDir: SUBDIRECTORY, destination: SUBDIRECTORY}],
},
})

const {include} = (spec.clientSteps[0]!.steps[0]!.config as {inclusions: [{include: string[]}]}).inclusions[0]

expect(include).toEqual(expect.arrayContaining(['**/*.json', '**/*.toml', '**/*.yaml', '**/*.yml', '**/*.svg']))
})

test('config is serializable to JSON', () => {
const serialized = JSON.stringify(spec.clientSteps)
const deserialized = JSON.parse(serialized)

expect(deserialized[0].steps).toHaveLength(1)
expect(deserialized[0].steps[0].config.inclusions[0].type).toBe('pattern')
})
})

describe('build integration', () => {
test('copies specification files to output, preserving subdirectory structure', async () => {
await inTemporaryDirectory(async (tmpDir) => {
// Given
const extensionDir = joinPath(tmpDir, 'extension')
const specsDir = joinPath(extensionDir, SUBDIRECTORY)
const outputDir = joinPath(tmpDir, 'output')

await mkdir(specsDir)
await mkdir(outputDir)

await writeFile(joinPath(specsDir, 'product.json'), '{}')
await writeFile(joinPath(specsDir, 'order.toml'), '[spec]')
await writeFile(joinPath(specsDir, 'logo.svg'), '<svg/>')
// Root-level files should NOT be copied
await writeFile(joinPath(extensionDir, 'README.md'), '# readme')
await writeFile(joinPath(extensionDir, 'index.js'), 'ignored')

const extension = new ExtensionInstance({
configuration: {name: 'my-channel', type: 'channel'},
configurationPath: '',
directory: extensionDir,
specification: spec,
})
extension.outputPath = outputDir

const buildOptions: ExtensionBuildOptions = {
stdout: new Writable({
write(chunk, enc, cb) {
cb()
},
}),
stderr: new Writable({
write(chunk, enc, cb) {
cb()
},
}),
app: {} as any,
environment: 'production',
}

// When
await extension.build(buildOptions)

// Then — specification files copied with path preserved
await expect(fileExists(joinPath(outputDir, SUBDIRECTORY, 'product.json'))).resolves.toBe(true)
await expect(fileExists(joinPath(outputDir, SUBDIRECTORY, 'order.toml'))).resolves.toBe(true)
await expect(fileExists(joinPath(outputDir, SUBDIRECTORY, 'logo.svg'))).resolves.toBe(true)

// Root-level files not in specifications/ are not copied
await expect(fileExists(joinPath(outputDir, 'README.md'))).resolves.toBe(false)
await expect(fileExists(joinPath(outputDir, 'index.js'))).resolves.toBe(false)
})
})

test('does not copy files with non-matching extensions inside specifications/', async () => {
await inTemporaryDirectory(async (tmpDir) => {
// Given
const extensionDir = joinPath(tmpDir, 'extension')
const specsDir = joinPath(extensionDir, SUBDIRECTORY)
const outputDir = joinPath(tmpDir, 'output')

await mkdir(specsDir)
await mkdir(outputDir)

await writeFile(joinPath(specsDir, 'spec.json'), '{}')
await writeFile(joinPath(specsDir, 'ignored.ts'), 'const x = 1')
await writeFile(joinPath(specsDir, 'ignored.js'), 'const x = 1')

const extension = new ExtensionInstance({
configuration: {name: 'my-channel', type: 'channel'},
configurationPath: '',
directory: extensionDir,
specification: spec,
})
extension.outputPath = outputDir

const buildOptions: ExtensionBuildOptions = {
stdout: new Writable({
write(chunk, enc, cb) {
cb()
},
}),
stderr: new Writable({
write(chunk, enc, cb) {
cb()
},
}),
app: {} as any,
environment: 'production',
}

// When
await extension.build(buildOptions)

// Then
await expect(fileExists(joinPath(outputDir, SUBDIRECTORY, 'spec.json'))).resolves.toBe(true)
await expect(fileExists(joinPath(outputDir, SUBDIRECTORY, 'ignored.ts'))).resolves.toBe(false)
await expect(fileExists(joinPath(outputDir, SUBDIRECTORY, 'ignored.js'))).resolves.toBe(false)
})
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,28 @@ const channelSpecificationSpec = createContractBasedModuleSpecification({
mode: 'copy_files',
filePatterns: FILE_EXTENSIONS.map((ext) => joinPath(SUBDIRECTORY_NAME, '**', `*.${ext}`)),
},
clientSteps: [
{
lifecycle: 'deploy',
steps: [
{
id: 'copy-files',
name: 'Copy Files',
type: 'include_assets',
config: {
inclusions: [
{
type: 'pattern',
baseDir: SUBDIRECTORY_NAME,
destination: SUBDIRECTORY_NAME,
include: FILE_EXTENSIONS.map((ext) => `**/*.${ext}`),
},
],
},
},
],
},
],
appModuleFeatures: () => [],
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ const checkoutPostPurchaseSpec = createExtensionSpecification({
schema: CheckoutPostPurchaseSchema,
appModuleFeatures: (_) => ['ui_preview', 'cart_url', 'esbuild', 'single_js_entry_path'],
buildConfig: {mode: 'ui'},
clientSteps: [
{
lifecycle: 'deploy',
steps: [
{id: 'bundle-ui', name: 'Bundle UI Extension', type: 'bundle_ui', config: {}},
{id: 'copy-static-assets', name: 'Copy Static Assets', type: 'copy_static_assets', config: {}},
],
},
],
deployConfig: async (config, _) => {
return {metafields: config.metafields ?? []}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@ const checkoutSpec = createExtensionSpecification({
schema: CheckoutSchema,
appModuleFeatures: (_) => ['ui_preview', 'cart_url', 'esbuild', 'single_js_entry_path', 'generates_source_maps'],
buildConfig: {mode: 'ui'},
clientSteps: [
{
lifecycle: 'deploy',
steps: [
{id: 'bundle-ui', name: 'Bundle UI Extension', type: 'bundle_ui', config: {}},
{id: 'copy-static-assets', name: 'Copy Static Assets', type: 'copy_static_assets', config: {}},
],
},
],
deployConfig: async (config, directory) => {
return {
extension_points: config.extension_points,
Expand Down
Loading
Loading