refactor: remove buildConfig from specifications in favor of clientSteps#7334
Conversation
The buildConfig property on extension specifications was redundant with clientSteps. This removes the BuildConfig type, the property from ExtensionSpecification, and all usages across specification files. Consumer sites in deploy.ts and extension-instance.ts now use clientSteps-based checks (hasDeploySteps getter) instead of buildConfig.mode comparisons. bundleAndBuildExtensions now always creates the bundle directory but only compresses and returns a bundlePath when extensions have deploy steps, avoiding unnecessary GCS uploads for manifest-only bundles. Co-authored-by: Isaac Roldán <isaac.roldan@shopify.com>
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
This PR refactors extension deploy bundling to remove the legacy buildConfig contract from extension specifications and rely on clientSteps (and a new hasDeploySteps signal) to decide when a deploy bundle should be created and uploaded.
Changes:
- Removed
buildConfigfromExtensionSpecificationand from all affected extension specs. - Added
ExtensionInstance.hasDeployStepsand updated bundling logic to key off deploy lifecycle steps. - Updated deploy flow so
bundleAndBuildExtensionsreturns an optional bundle path (upload skipped when undefined).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/app/src/cli/services/deploy/bundle.ts | Makes bundle creation conditional and returns bundlePath only when a bundle should be uploaded. |
| packages/app/src/cli/services/deploy.ts | Simplifies deploy bundling setup and consumes optional bundlePath. |
| packages/app/src/cli/models/extensions/specifications/web_pixel_extension.ts | Removes buildConfig in favor of existing clientSteps. |
| packages/app/src/cli/models/extensions/specifications/ui_extension.ts | Removes buildConfig in favor of existing clientSteps. |
| packages/app/src/cli/models/extensions/specifications/theme.ts | Removes buildConfig in favor of existing clientSteps. |
| packages/app/src/cli/models/extensions/specifications/tax_calculation.ts | Removes buildConfig in favor of existing clientSteps. |
| packages/app/src/cli/models/extensions/specifications/product_subscription.ts | Removes buildConfig in favor of existing clientSteps. |
| packages/app/src/cli/models/extensions/specifications/pos_ui_extension.ts | Removes buildConfig in favor of existing clientSteps. |
| packages/app/src/cli/models/extensions/specifications/function.ts | Removes buildConfig in favor of existing clientSteps. |
| packages/app/src/cli/models/extensions/specifications/flow_template.ts | Removes buildConfig in favor of existing clientSteps. |
| packages/app/src/cli/models/extensions/specifications/checkout_ui_extension.ts | Removes buildConfig in favor of existing clientSteps. |
| packages/app/src/cli/models/extensions/specifications/checkout_post_purchase.ts | Removes buildConfig in favor of existing clientSteps. |
| packages/app/src/cli/models/extensions/specifications/channel.ts | Removes buildConfig copy-files config; uses include_assets client step instead. |
| packages/app/src/cli/models/extensions/specifications/admin.ts | Removes buildConfig copy-files config; relies on include_assets client step. |
| packages/app/src/cli/models/extensions/specification.ts | Deletes BuildConfig type and buildConfig property from spec interfaces/builders. |
| packages/app/src/cli/models/extensions/extension-instance.ts | Adds hasDeploySteps and switches bundling decisions from buildConfig to clientSteps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (this.isThemeExtension) { | ||
| await bundleThemeExtension(this, options) | ||
| } else if (buildMode !== 'none') { | ||
| } else if (this.hasDeploySteps) { | ||
| outputDebug(`Will copy pre-built file from ${defaultOutputPath} to ${this.outputPath}`) | ||
| if (await fileExists(defaultOutputPath)) { | ||
| await copyFile(defaultOutputPath, this.outputPath) | ||
|
|
||
| if (buildMode === 'function') { | ||
| if (this.isFunctionExtension) { | ||
| await bundleFunctionExtension(this.outputPath, this.outputPath) |
There was a problem hiding this comment.
copyIntoBundle assumes a single file output and uses copyFile(defaultOutputPath, this.outputPath). For extensions whose outputPath is a directory (e.g., specs that only run include_assets and don't define getOutputRelativePath), fileExists(defaultOutputPath) will be true and copyFile will throw (EISDIR). It also won't include any additional files placed alongside the main output (e.g., static assets copied into the output directory). Consider handling directory outputs (recursive copy) and/or executing the relevant non-build client steps (like include_assets/copy_static_assets) even when skipBuild is enabled.
| /** | ||
| * Builds all extensions into a bundle directory and compresses it if any | ||
| * extension produced output. Returns the bundlePath if a bundle was created, | ||
| * or undefined if only the manifest was present (nothing to upload). | ||
| */ | ||
| export async function bundleAndBuildExtensions(options: BundleOptions): Promise<string | undefined> { |
There was a problem hiding this comment.
The docstring says this returns undefined when “only the manifest was present (nothing to upload)”, but the decision is currently based on ext.hasDeploySteps, not on whether any files beyond manifest.json were actually written into the bundle directory. This can lead to returning a bundle path (and uploading) even when deploy steps copied 0 files. Consider basing this on bundle directory contents (excluding manifest.json) or tracking whether any step produced output.
| const hasExtensionOutput = options.app.allExtensions.some((ext) => ext.hasDeploySteps) | ||
|
|
||
| if (hasExtensionOutput) { | ||
| await compressBundle(bundleDirectory, options.bundlePath) | ||
| return options.bundlePath | ||
| } | ||
|
|
||
| return undefined |
There was a problem hiding this comment.
bundleAndBuildExtensions now returns string | undefined and can skip compression when hasExtensionOutput is false. There are existing tests for bundling with UI/function/theme extensions, but no test covering the new “no deploy steps / manifest-only” branch to assert that the function returns undefined and does not create the bundle file.
WHY are these changes introduced?
Fixes #0000
WHAT is this pull request doing?
How to test your changes?
Post-release steps
Checklist
pnpm changeset add