Skip to content
Merged
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
5 changes: 5 additions & 0 deletions .changeset/nervous-pandas-acknowledge.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/app': minor
---

Show a confirmation when configuration-only extensions are accepted during `shopify app dev`
203 changes: 202 additions & 1 deletion packages/app/src/cli/models/extensions/extension-instance.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import {SingleWebhookSubscriptionType} from './specifications/app_config_webhook_schemas/webhooks_schema.js'
import {MAX_EXTENSION_HANDLE_LENGTH} from './schemas.js'
import {DEFAULT_DEV_SESSION_UPDATE_MESSAGE, ExtensionInstance} from './extension-instance.js'
import {
ExtensionSpecification,
createConfigExtensionSpecification,
createContractBasedModuleSpecification,
createExtensionSpecification,
} from './specification.js'
import {loadLocalExtensionsSpecifications} from './load-specifications.js'
import {BaseConfigType, BaseSchema, MAX_EXTENSION_HANDLE_LENGTH} from './schemas.js'
import {FunctionConfigType} from './specifications/function.js'
import {
testApp,
Expand All @@ -15,6 +23,7 @@ import {
placeholderAppConfiguration,
} from '../app/app.test-data.js'
import {ExtensionBuildOptions} from '../../services/build/extension.js'
import {ClientSteps} from '../../services/build/client-steps.js'
import {DeveloperPlatformClient} from '../../utilities/developer-platform-client.js'
import {joinPath} from '@shopify/cli-kit/node/path'
import {describe, expect, test, vi} from 'vitest'
Expand Down Expand Up @@ -714,3 +723,195 @@ describe('SHOPIFY_CLI_DISABLE_IMPORT_SCANNING', () => {
})
})
})

describe('getDevSessionUpdateMessages', () => {
const deployStep: ClientSteps = [
{lifecycle: 'deploy', steps: [{id: 'build-theme', name: 'Build Theme', type: 'build_theme'}]},
]

function instanceFor(specification: ExtensionSpecification): ExtensionInstance {
return new ExtensionInstance({
configuration: {name: 'test extension', type: specification.identifier} as BaseConfigType,
configurationPath: '',
directory: '/tmp/test-extension',
specification,
})
}

function specWithNoLocalDevOutput(identifier = 'no_local_dev_output') {
return createExtensionSpecification({identifier, schema: BaseSchema, appModuleFeatures: () => []})
}

test('returns the default message for a module with no local dev output on the first dev session', async () => {
const extensionInstance = instanceFor(specWithNoLocalDevOutput())

const got = await extensionInstance.getDevSessionUpdateMessages({status: 'created'})

expect(got).toEqual([DEFAULT_DEV_SESSION_UPDATE_MESSAGE])
})

test('returns nothing for a module with no local dev output on subsequent updates', async () => {
const extensionInstance = instanceFor(specWithNoLocalDevOutput())

const got = await extensionInstance.getDevSessionUpdateMessages({status: 'updated'})

expect(got).toBeUndefined()
})

test('returns nothing when the module contributes features', async () => {
const extensionInstance = instanceFor(
createExtensionSpecification({
identifier: 'has_features',
schema: BaseSchema,
appModuleFeatures: () => ['localization'],
}),
)

expect(extensionInstance.hasNoLocalDevOutput).toBe(false)
await expect(extensionInstance.getDevSessionUpdateMessages({status: 'created'})).resolves.toBeUndefined()
})

test('returns nothing when the module has deploy steps', async () => {
const extensionInstance = instanceFor(
createExtensionSpecification({
identifier: 'has_deploy_steps',
schema: BaseSchema,
appModuleFeatures: () => [],
clientSteps: deployStep,
}),
)

expect(extensionInstance.hasNoLocalDevOutput).toBe(false)
await expect(extensionInstance.getDevSessionUpdateMessages({status: 'created'})).resolves.toBeUndefined()
})

test('returns nothing when the module produces build output', async () => {
const extensionInstance = instanceFor(
createExtensionSpecification({
identifier: 'has_build_output',
schema: BaseSchema,
appModuleFeatures: () => [],
getOutputRelativePath: () => 'dist/main.js',
}),
)

expect(extensionInstance.hasNoLocalDevOutput).toBe(false)
await expect(extensionInstance.getDevSessionUpdateMessages({status: 'created'})).resolves.toBeUndefined()
})

test('returns nothing for app config modules, which are summarised together instead', async () => {
const extensionInstance = instanceFor(
createConfigExtensionSpecification({
identifier: 'app_config_without_messages',
schema: BaseSchema,
transformConfig: {},
}),
)

expect(extensionInstance.isAppConfigExtension).toBe(true)
expect(extensionInstance.hasNoLocalDevOutput).toBe(true)
await expect(extensionInstance.getDevSessionUpdateMessages({status: 'created'})).resolves.toBeUndefined()
})

test('evaluates the app config exclusion lazily, so a remotely-rewritten experience is respected', async () => {
const specification = specWithNoLocalDevOutput()
const extensionInstance = instanceFor({...specification, experience: 'configuration'})

await expect(extensionInstance.getDevSessionUpdateMessages({status: 'created'})).resolves.toBeUndefined()
})

describe('per-spec override', () => {
const override = async () => ['Custom message']

test('wins over the default through createExtensionSpecification', async () => {
const extensionInstance = instanceFor(
createExtensionSpecification({
identifier: 'override_via_extension_spec',
schema: BaseSchema,
appModuleFeatures: () => [],
getDevSessionUpdateMessages: override,
}),
)

await expect(extensionInstance.getDevSessionUpdateMessages({status: 'created'})).resolves.toEqual([
'Custom message',
])
})

test('wins over the default through createConfigExtensionSpecification', async () => {
const extensionInstance = instanceFor(
createConfigExtensionSpecification({
identifier: 'override_via_config_spec',
schema: BaseSchema,
transformConfig: {},
getDevSessionUpdateMessages: override,
}),
)

await expect(extensionInstance.getDevSessionUpdateMessages({status: 'created'})).resolves.toEqual([
'Custom message',
])
})

test('wins over the default when spread onto a contract based module specification', async () => {
const specification = createContractBasedModuleSpecification({
identifier: 'override_via_contract_based_spec',
experience: 'extension',
uidStrategy: 'single',
appModuleFeatures: () => [],
})
const extensionInstance = instanceFor({...specification, getDevSessionUpdateMessages: override})

await expect(extensionInstance.getDevSessionUpdateMessages({status: 'created'})).resolves.toEqual([
'Custom message',
])
})

test('is used even on subsequent updates, where the default stays quiet', async () => {
const extensionInstance = instanceFor(
createExtensionSpecification({
identifier: 'override_on_update',
schema: BaseSchema,
appModuleFeatures: () => [],
getDevSessionUpdateMessages: override,
}),
)

await expect(extensionInstance.getDevSessionUpdateMessages({status: 'updated'})).resolves.toEqual([
'Custom message',
])
})

test('receives the dev session context', async () => {
const getDevSessionUpdateMessages = vi.fn().mockResolvedValue([])
const extensionInstance = instanceFor(
createExtensionSpecification({
identifier: 'override_receiving_context',
schema: BaseSchema,
appModuleFeatures: () => [],
getDevSessionUpdateMessages,
}),
)

await extensionInstance.getDevSessionUpdateMessages({status: 'created'})

expect(getDevSessionUpdateMessages).toHaveBeenCalledWith(extensionInstance.configuration, {status: 'created'})
})
})

describe('local specifications matching the default', () => {
test('only modules with no local dev output receive the default message', async () => {
const specifications = await loadLocalExtensionsSpecifications()

const matching = specifications
.filter((specification) => {
const extensionInstance = instanceFor(specification)
return !extensionInstance.isAppConfigExtension && extensionInstance.hasNoLocalDevOutput
})
.map((specification) => specification.identifier)
.sort()

expect(matching).toEqual(['editor_extension_collection', 'flow_action', 'flow_trigger', 'payments_extension'])
})
})
})
26 changes: 22 additions & 4 deletions packages/app/src/cli/models/extensions/extension-instance.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import {BaseConfigType, MAX_EXTENSION_HANDLE_LENGTH, MAX_UID_LENGTH} from './schemas.js'
import {FunctionConfigType} from './specifications/function.js'
import {DevSessionWatchConfig, ExtensionFeature, ExtensionSpecification} from './specification.js'
import {
DevSessionUpdateContext,
DevSessionWatchConfig,
ExtensionFeature,
ExtensionSpecification,
} from './specification.js'
import {SingleWebhookSubscriptionType} from './specifications/app_config_webhook_schemas/webhooks_schema.js'
import {ExtensionBuildOptions} from '../../services/build/extension.js'
import {ExtensionUuidsByLocalIdentifier} from '../app/identifiers.js'
Expand Down Expand Up @@ -39,6 +44,8 @@ const DEFAULT_WATCH_IGNORE = [
'**/.gitignore',
]

export const DEFAULT_DEV_SESSION_UPDATE_MESSAGE = 'Configuration accepted'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find accepted a bit weird... What about ready or active?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the message makes sense, it means you have a valid configuration and we didn't hit any errors pushing it up


/**
* Class that represents an instance of a local extension
* Before creating this class we've validated that:
Expand Down Expand Up @@ -143,6 +150,10 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi
return this.specification.getOutputRelativePath?.(this) ?? ''
}

get hasNoLocalDevOutput(): boolean {
return this.features.length === 0 && !this.hasDeploySteps && this.outputRelativePath === ''
}

constructor(options: {
configuration: TConfiguration
configurationPath: string
Expand Down Expand Up @@ -394,9 +405,16 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi
}
}

async getDevSessionUpdateMessages(): Promise<string[] | undefined> {
if (!this.specification.getDevSessionUpdateMessages) return undefined
return this.specification.getDevSessionUpdateMessages(this.configuration)
async getDevSessionUpdateMessages(context: DevSessionUpdateContext): Promise<string[] | undefined> {
if (this.specification.getDevSessionUpdateMessages) {
return this.specification.getDevSessionUpdateMessages(this.configuration, context)
}

if (context.status !== 'created') return undefined
if (this.isAppConfigExtension) return undefined
if (!this.hasNoLocalDevOutput) return undefined

return [DEFAULT_DEV_SESSION_UPDATE_MESSAGE]
}

/**
Expand Down
10 changes: 8 additions & 2 deletions packages/app/src/cli/models/extensions/specification.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ interface ExtensionDeployConfigContext {
appConfiguration: AppConfiguration
}

export type DevSessionUpdateStatus = 'created' | 'updated'

export interface DevSessionUpdateContext {
status: DevSessionUpdateStatus
}

/**
* Extension specification with all the needed properties and methods to load an extension.
*/
Expand Down Expand Up @@ -91,7 +97,7 @@ export interface ExtensionSpecification<TConfiguration extends BaseConfigType =
buildValidation?: (extension: ExtensionInstance<TConfiguration>, outputPath: string) => Promise<void>
hasExtensionPointTarget?(config: TConfiguration, target: string): boolean
appModuleFeatures: (config?: TConfiguration) => ExtensionFeature[]
getDevSessionUpdateMessages?: (config: TConfiguration) => Promise<string[]>
getDevSessionUpdateMessages?: (config: TConfiguration, context: DevSessionUpdateContext) => Promise<string[]>
patchWithAppDevURLs?: (config: TConfiguration, urls: ApplicationURLs) => void

/**
Expand Down Expand Up @@ -271,7 +277,7 @@ export function createConfigExtensionSpecification<TConfiguration extends BaseCo
appModuleFeatures?: (config?: TConfiguration) => ExtensionFeature[]
transformConfig: TransformationConfig | CustomTransformationConfig
uidStrategy?: UidStrategy
getDevSessionUpdateMessages?: (config: TConfiguration) => Promise<string[]>
getDevSessionUpdateMessages?: (config: TConfiguration, context: DevSessionUpdateContext) => Promise<string[]>
patchWithAppDevURLs?: (config: TConfiguration, urls: ApplicationURLs) => void
}): ExtensionSpecification<TConfiguration> {
const appModuleFeatures = spec.appModuleFeatures ?? (() => [])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ describe('app_config_app_access', () => {
}

// When
const result = await spec.getDevSessionUpdateMessages!(config)
const result = await spec.getDevSessionUpdateMessages!(config, {status: 'created'})

// Then
expect(result).toEqual(['Access scopes auto-granted: read_products, write_products'])
Expand All @@ -106,7 +106,7 @@ describe('app_config_app_access', () => {
}

// When
const result = await spec.getDevSessionUpdateMessages!(config)
const result = await spec.getDevSessionUpdateMessages!(config, {status: 'created'})

// Then
expect(result).toEqual(['Access scopes auto-granted: write_orders, read_inventory'])
Expand All @@ -121,7 +121,7 @@ describe('app_config_app_access', () => {
}

// When
const result = await spec.getDevSessionUpdateMessages!(config)
const result = await spec.getDevSessionUpdateMessages!(config, {status: 'created'})

// Then
expect(result).toEqual(['App has been installed'])
Expand All @@ -140,7 +140,7 @@ describe('app_config_app_access', () => {
}

// When
const result = await spec.getDevSessionUpdateMessages!(config)
const result = await spec.getDevSessionUpdateMessages!(config, {status: 'created'})

// Then
expect(result).toEqual(['Using legacy install flow - access scopes are not auto-granted'])
Expand All @@ -159,7 +159,7 @@ describe('app_config_app_access', () => {
}

// When
const result = await spec.getDevSessionUpdateMessages!(config)
const result = await spec.getDevSessionUpdateMessages!(config, {status: 'created'})

// Then
expect(result).toEqual(['Using legacy install flow - access scopes are not auto-granted'])
Expand All @@ -178,7 +178,7 @@ describe('app_config_app_access', () => {
}

// When
const result = await spec.getDevSessionUpdateMessages!(config)
const result = await spec.getDevSessionUpdateMessages!(config, {status: 'created'})

// Then
expect(result).toEqual(['Access scopes auto-granted: read_products, write_products'])
Expand All @@ -194,7 +194,7 @@ describe('app_config_app_access', () => {
}

// When
const result = await spec.getDevSessionUpdateMessages!(config)
const result = await spec.getDevSessionUpdateMessages!(config, {status: 'created'})

// Then
expect(result).toEqual(['Using legacy install flow - access scopes are not auto-granted'])
Expand All @@ -212,7 +212,7 @@ describe('app_config_app_access', () => {
}

// When
const result = await spec.getDevSessionUpdateMessages!(config)
const result = await spec.getDevSessionUpdateMessages!(config, {status: 'created'})

// Then
expect(result).toEqual(['App has been installed'])
Expand All @@ -230,7 +230,7 @@ describe('app_config_app_access', () => {
}

// When
const result = await spec.getDevSessionUpdateMessages!(config)
const result = await spec.getDevSessionUpdateMessages!(config, {status: 'created'})

// Then
expect(result).toEqual(['App has been installed'])
Expand Down
Loading
Loading