Skip to content

Rename extension-to-toml → extension-config-builder #6945

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.

Sign up for GitHub

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

Merged
ryancbahan merged 2 commits into from
Mar 11, 2026
Merged

Rename extension-to-toml → extension-config-builder #6945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea56708
Rename extension-to-toml → extension-config-builder, buildTomlObject …
ryancbahan Mar 6, 2026
eefb2a9
Update test descriptions from 'toml object' to 'config object'
ryancbahan Mar 11, 2026
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
2 changes: 1 addition & 1 deletion packages/app/src/cli/commands/app/import-extensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export default class ImportExtensions extends AppLinkedCommand {
...appContext,
extensions,
extensionTypes: migrationChoice.extensionTypes,
buildTomlObject: migrationChoice.buildTomlObject,
buildExtensionConfig: migrationChoice.buildExtensionConfig,
})
}

Expand Down
20 changes: 10 additions & 10 deletions packages/app/src/cli/prompts/import-extensions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ describe('allMigrationChoices', () => {
expect(choice).toHaveProperty('label')
expect(choice).toHaveProperty('value')
expect(choice).toHaveProperty('extensionTypes')
expect(choice).toHaveProperty('buildTomlObject')
expect(choice).toHaveProperty('buildExtensionConfig')
expect(Array.isArray(choice.extensionTypes)).toBe(true)
expect(choice.extensionTypes.length).toBeGreaterThan(0)
expect(typeof choice.buildTomlObject).toBe('function')
expect(typeof choice.buildExtensionConfig).toBe('function')
})
})

Expand Down Expand Up @@ -134,7 +134,7 @@ describe('selectMigrationChoice', () => {
label: 'Test Extension',
value: 'test',
extensionTypes: ['test_type'],
buildTomlObject: vi.fn(),
buildExtensionConfig: vi.fn(),
}
const result = await selectMigrationChoice([singleChoice])
expect(result).toBe(singleChoice)
Expand All @@ -147,13 +147,13 @@ describe('selectMigrationChoice', () => {
label: 'Choice 1',
value: 'choice1',
extensionTypes: ['type1'],
buildTomlObject: vi.fn(),
buildExtensionConfig: vi.fn(),
},
{
label: 'Choice 2',
value: 'choice2',
extensionTypes: ['type2'],
buildTomlObject: vi.fn(),
buildExtensionConfig: vi.fn(),
},
]

Expand All @@ -177,13 +177,13 @@ describe('selectMigrationChoice', () => {
label: 'Choice 1',
value: 'choice1',
extensionTypes: ['type1'],
buildTomlObject: vi.fn(),
buildExtensionConfig: vi.fn(),
},
{
label: 'Choice 2',
value: 'choice2',
extensionTypes: ['type2'],
buildTomlObject: vi.fn(),
buildExtensionConfig: vi.fn(),
},
]

Expand All @@ -204,19 +204,19 @@ describe('selectMigrationChoice', () => {
label: 'Payments Extensions',
value: 'payments',
extensionTypes: ['payments_app'],
buildTomlObject: vi.fn(),
buildExtensionConfig: vi.fn(),
},
{
label: 'Flow Extensions',
value: 'flow',
extensionTypes: ['flow_action_definition'],
buildTomlObject: vi.fn(),
buildExtensionConfig: vi.fn(),
},
{
label: 'Marketing Activity Extensions',
value: 'marketing activity',
extensionTypes: ['marketing_activity_extension'],
buildTomlObject: vi.fn(),
buildExtensionConfig: vi.fn(),
},
]

Expand Down
22 changes: 11 additions & 11 deletions packages/app/src/cli/prompts/import-extensions.ts
View file
Open in desktop
Copy link
Contributor

@dmerand dmerand Mar 10, 2026

Choose a reason for hiding this comment

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

⭐ Praise: The renaming from buildTomlObject to buildExtensionConfig has been applied consistently across function definitions, interface properties, and all callsites throughout the codebase. The new import aliases (buildPaymentsConfig, buildFlowConfig, etc.) are more concise and clearly convey that these functions build configuration objects. This is solid refactoring work.

Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import {buildTomlObject as buildPaymentsTomlObject} from '../services/payments/extension-to-toml.js'
import {buildTomlObject as buildFlowTomlObject} from '../services/flow/extension-to-toml.js'
import {buildTomlObject as buildAdminLinkTomlObject} from '../services/admin-link/extension-to-toml.js'
import {buildTomlObject as buildMarketingActivityTomlObject} from '../services/marketing_activity/extension-to-toml.js'
import {buildTomlObject as buildSubscriptionLinkTomlObject} from '../services/subscription_link/extension-to-toml.js'
import {buildExtensionConfig as buildPaymentsConfig} from '../services/payments/extension-config-builder.js'
import {buildExtensionConfig as buildFlowConfig} from '../services/flow/extension-config-builder.js'
import {buildExtensionConfig as buildAdminLinkConfig} from '../services/admin-link/extension-config-builder.js'
import {buildExtensionConfig as buildMarketingActivityConfig} from '../services/marketing_activity/extension-config-builder.js'
import {buildExtensionConfig as buildSubscriptionLinkConfig} from '../services/subscription_link/extension-config-builder.js'
import {ExtensionRegistration} from '../api/graphql/all_app_extension_registrations.js'
import {CurrentAppConfiguration} from '../models/app/app.js'
import {AbortError} from '@shopify/cli-kit/node/error'
Expand All @@ -12,7 +12,7 @@ export interface MigrationChoice {
label: string
value: string
extensionTypes: string[]
buildTomlObject: (
buildExtensionConfig: (
ext: ExtensionRegistration,
allExtensions: ExtensionRegistration[],
appConfiguration: CurrentAppConfiguration,
Expand All @@ -31,31 +31,31 @@ export const allMigrationChoices: MigrationChoice[] = [
'payments_app_redeemable',
'payments_extension',
],
buildTomlObject: buildPaymentsTomlObject,
buildExtensionConfig: buildPaymentsConfig,
},
{
label: 'Flow Extensions',
value: 'flow',
extensionTypes: ['flow_action_definition', 'flow_trigger_definition', 'flow_trigger_discovery_webhook'],
buildTomlObject: buildFlowTomlObject,
buildExtensionConfig: buildFlowConfig,
},
{
label: 'Marketing Activity Extensions',
value: 'marketing activity',
extensionTypes: ['marketing_activity_extension'],
buildTomlObject: buildMarketingActivityTomlObject,
buildExtensionConfig: buildMarketingActivityConfig,
},
{
label: 'Subscription Link Extensions',
value: 'subscription link',
extensionTypes: ['subscription_link', 'subscription_link_extension'],
buildTomlObject: buildSubscriptionLinkTomlObject,
buildExtensionConfig: buildSubscriptionLinkConfig,
},
{
label: 'Admin Link extensions',
value: 'link extension',
extensionTypes: ['app_link', 'bulk_action'],
buildTomlObject: buildAdminLinkTomlObject,
buildExtensionConfig: buildAdminLinkConfig,
},
]

Expand Down
20 changes: 10 additions & 10 deletions ...ices/admin-link/extension-to-toml.test.ts → ...min-link/extension-config-builder.test.ts
View file
Open in desktop
Copy link
Contributor

@dmerand dmerand Mar 10, 2026

Choose a reason for hiding this comment

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

🎨 Code Style: Multiple test descriptions in this file state 'correctly builds a toml object' (lines 6, 48, 88, 128). The function being tested now builds and returns a configuration object, not TOML. The test descriptions should be updated to say 'correctly builds a config object' or 'correctly builds an extension config' to align with the rename from buildTomlObject to buildExtensionConfig.

Suggestion: Update all test descriptions from 'correctly builds a toml object for...' to 'correctly builds a config object for...' to maintain consistency with the refactoring's stated goal.

Copy link
Contributor Author

@ryancbahan ryancbahan Mar 11, 2026

Choose a reason for hiding this comment

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

mirroring existing behavior -- happy to change as separate pr, but I'm trying to reduce surface area given this is a large change. the syntax here is pre-existing pattern

Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import {buildTomlObject} from './extension-to-toml.js'
import {buildExtensionConfig} from './extension-config-builder.js'
import {ExtensionRegistration} from '../../api/graphql/all_app_extension_registrations.js'
import {describe, expect, test} from 'vitest'

describe('extension-to-toml', () => {
test('correctly builds a toml object for a app_link extension on a non embedded app', () => {
describe('extension-config-builder', () => {
test('correctly builds a config object for a app_link extension on a non embedded app', () => {
// Given
const appConfig = {
path: '',
Expand All @@ -25,7 +25,7 @@ describe('extension-to-toml', () => {
}

// When
const got = buildTomlObject(extension1, [], appConfig)
const got = buildExtensionConfig(extension1, [], appConfig)

// Then
expect(got).toEqual({
Expand All @@ -45,7 +45,7 @@ describe('extension-to-toml', () => {
})
})

test('correctly builds a toml object for bulk_action extension with path in an embedded app', () => {
test('correctly builds a config object for bulk_action extension with path in an embedded app', () => {
// Given
const appConfig = {
path: '',
Expand All @@ -66,7 +66,7 @@ describe('extension-to-toml', () => {
}

// When
const got = buildTomlObject(extension1, [], appConfig)
const got = buildExtensionConfig(extension1, [], appConfig)

// Then
expect(got).toEqual({
Expand All @@ -85,7 +85,7 @@ describe('extension-to-toml', () => {
],
})
})
test('correctly builds a toml object for bulk_action extension with no path in an embedded app', () => {
test('correctly builds a config object for bulk_action extension with no path in an embedded app', () => {
// Given
const appConfig = {
path: '',
Expand All @@ -106,7 +106,7 @@ describe('extension-to-toml', () => {
}

// When
const got = buildTomlObject(extension1, [], appConfig)
const got = buildExtensionConfig(extension1, [], appConfig)

// Then
expect(got).toEqual({
Expand All @@ -125,7 +125,7 @@ describe('extension-to-toml', () => {
],
})
})
test('correctly builds a toml object for bulk_action extension with no path but search query in an embedded app', () => {
test('correctly builds a config object for bulk_action extension with no path but search query in an embedded app', () => {
// Given
const appConfig = {
path: '',
Expand All @@ -146,7 +146,7 @@ describe('extension-to-toml', () => {
}

// When
const got = buildTomlObject(extension1, [], appConfig)
const got = buildExtensionConfig(extension1, [], appConfig)

// Then
expect(got).toEqual({
Expand Down
2 changes: 1 addition & 1 deletion .../services/admin-link/extension-to-toml.ts → ...es/admin-link/extension-config-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ interface AdminLinkConfig {
/**
* Given an app_link or bulk_action extension config file, convert it to toml
*/
export function buildTomlObject(
export function buildExtensionConfig(
extension: ExtensionRegistration,
_: ExtensionRegistration[],
appConfiguration: CurrentAppConfiguration,
Expand Down
14 changes: 7 additions & 7 deletions ...i/services/flow/extension-to-toml.test.ts → ...ces/flow/extension-config-builder.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import {buildTomlObject} from './extension-to-toml.js'
import {buildExtensionConfig} from './extension-config-builder.js'
import {ExtensionRegistration} from '../../api/graphql/all_app_extension_registrations.js'
import {describe, expect, test} from 'vitest'

describe('extension-to-toml', () => {
test('correctly builds a toml object for a flow_action', () => {
describe('extension-config-builder', () => {
test('correctly builds a config object for a flow_action', () => {
// Given
const extension1: ExtensionRegistration = {
id: '26237698049',
Expand All @@ -17,7 +17,7 @@ describe('extension-to-toml', () => {
}

// When
const got = buildTomlObject(extension1)
const got = buildExtensionConfig(extension1)

// Then
expect(got).toEqual({
Expand Down Expand Up @@ -64,7 +64,7 @@ describe('extension-to-toml', () => {
}

// When
const got = buildTomlObject(extension1)
const got = buildExtensionConfig(extension1)

// Then
expect(got).toEqual({
Expand Down Expand Up @@ -97,7 +97,7 @@ describe('extension-to-toml', () => {
})
})

test('correctly builds a toml object for a flow_trigger', () => {
test('correctly builds a config object for a flow_trigger', () => {
// Given
const extension2 = {
id: '26237861889',
Expand All @@ -115,7 +115,7 @@ describe('extension-to-toml', () => {
}

// When
const got = buildTomlObject(extension2)
const got = buildExtensionConfig(extension2)

// Then
expect(got).toEqual({
Expand Down
2 changes: 1 addition & 1 deletion ...rc/cli/services/flow/extension-to-toml.ts → ...services/flow/extension-config-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ interface FlowWebhookConfig {
* Given a flow extension config file, convert it to toml
Copy link
Contributor

@dmerand dmerand Mar 10, 2026

Choose a reason for hiding this comment

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

🎨 Code Style: The comment states 'convert it to toml' but this function returns a configuration object. The TOML conversion is handled by the caller via TomlFile, not by this function. The comment should align with the refactored function naming.

Suggestion:

Suggested change
* Given a flow extension config file, convert it to toml
* Given a flow extension config file, builds the extension configuration object

* Works for both trigger and action because trigger config is a subset of action config
*/
export function buildTomlObject(extension: ExtensionRegistration): object {
export function buildExtensionConfig(extension: ExtensionRegistration): object {
const versionConfig = extension.activeVersion?.config ?? extension.draftVersion?.config
if (!versionConfig) throw new Error('No config found for extension')

Expand Down
14 changes: 7 additions & 7 deletions packages/app/src/cli/services/import-extensions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {importExtensions, filterOutImportedExtensions} from './import-extensions.js'
import {buildTomlObject} from './flow/extension-to-toml.js'
import {buildExtensionConfig} from './flow/extension-config-builder.js'
import {testAppLinked, testDeveloperPlatformClient, testUIExtension} from '../models/app/app.test-data.js'
import {OrganizationApp} from '../models/organization.js'
import {ExtensionRegistration} from '../api/graphql/all_app_extension_registrations.js'
Expand Down Expand Up @@ -110,7 +110,7 @@ describe('import-extensions', () => {
'subscription_link_extension',
],
extensions,
buildTomlObject,
buildExtensionConfig,
})

expect(renderSuccess).toHaveBeenCalledWith({
Expand Down Expand Up @@ -167,7 +167,7 @@ describe('import-extensions', () => {
developerPlatformClient: testDeveloperPlatformClient(),
extensionTypes: ['flow_action_definition'],
extensions,
buildTomlObject,
buildExtensionConfig,
})

// Then - expect the success message to be shown (even for skipped extensions)
Expand Down Expand Up @@ -213,7 +213,7 @@ describe('import-extensions', () => {
developerPlatformClient: testDeveloperPlatformClient(),
extensionTypes: ['flow_action_definition'],
extensions,
buildTomlObject,
buildExtensionConfig,
})

// Then - expect the success message to be shown
Expand Down Expand Up @@ -261,7 +261,7 @@ describe('import-extensions', () => {
developerPlatformClient: testDeveloperPlatformClient(),
extensionTypes: ['flow_action_definition'],
extensions,
buildTomlObject,
buildExtensionConfig,
}),
).rejects.toThrow(AbortSilentError)

Expand Down Expand Up @@ -300,7 +300,7 @@ describe('import-extensions', () => {
'subscription_link_extension',
],
extensions,
buildTomlObject,
buildExtensionConfig,
})

expect(renderSuccess).toHaveBeenCalledWith({
Expand Down Expand Up @@ -349,7 +349,7 @@ describe('import-extensions', () => {
'subscription_link_extension',
],
extensions,
buildTomlObject,
buildExtensionConfig,
}),
).rejects.toThrow('No extensions to migrate')

Expand Down
8 changes: 4 additions & 4 deletions packages/app/src/cli/services/import-extensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ interface ImportAllOptions {

interface ImportOptions extends ImportAllOptions {
extensionTypes: string[]
buildTomlObject: (
buildExtensionConfig: (
ext: ExtensionRegistration,
allExtensions: ExtensionRegistration[],
appConfig: CurrentAppConfiguration,
Expand Down Expand Up @@ -79,7 +79,7 @@ async function handleExtensionDirectory({
}

export async function importExtensions(options: ImportOptions) {
const {app, remoteApp, developerPlatformClient, extensionTypes, extensions, buildTomlObject, all} = options
const {app, remoteApp, developerPlatformClient, extensionTypes, extensions, buildExtensionConfig, all} = options

let extensionsToMigrate = extensions.filter((ext) => extensionTypes.includes(ext.type.toLowerCase()))
extensionsToMigrate = filterOutImportedExtensions(app, extensionsToMigrate)
Expand Down Expand Up @@ -111,7 +111,7 @@ export async function importExtensions(options: ImportOptions) {
extensionUuids[handle] = ext.uuid

if (action === DirectoryAction.Write) {
const tomlContent = buildTomlObject(ext, extensions, app.configuration)
const tomlContent = buildExtensionConfig(ext, extensions, app.configuration)
Copy link
Contributor

@dmerand dmerand Mar 10, 2026

Choose a reason for hiding this comment

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

💡 Improvement: The variable is named tomlContent but receives the result from buildExtensionConfig(), which returns a configuration object, not TOML content. The TOML conversion happens on the next line when creating the TomlFile. This naming is inconsistent with the PR's goal of reflecting that builders return config objects.

Suggestion: Consider renaming this variable to extensionConfig and updating the TomlFile initialization on the following lines accordingly:

Suggested change
const tomlContent = buildExtensionConfig(ext, extensions, app.configuration)
const extensionConfig = buildExtensionConfig(ext, extensions, app.configuration)

const tomlPath = joinPath(directory, 'shopify.extension.toml')
const file = new TomlFile(tomlPath, tomlContent as JsonMapType)
await file.replace(tomlContent as JsonMapType)
Expand Down Expand Up @@ -150,7 +150,7 @@ export async function importAllExtensions(options: ImportAllOptions) {
return importExtensions({
...options,
extensionTypes: choice.extensionTypes,
buildTomlObject: choice.buildTomlObject,
buildExtensionConfig: choice.buildExtensionConfig,
all: true,
})
}),
Expand Down
Loading
Loading