From 1777ba040ed4f73e3268efa5eb9e7eca0102b5f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Tue, 17 Mar 2026 11:22:57 +0100 Subject: [PATCH] Clean up ExtensionSpecification spreads and mutations Replace object spread patterns and direct mutations with class methods: - Add applyRemoteSpecification() to apply remote API overrides - Add markAsRemoteLoaded() for tests needing the type marker - Split unifiedConfigurationParserFactory params (spec + remoteSpec) - Pass uidStrategy at construction via createContractBasedModuleSpecification - Remove leaked remote fields (name) from test assertions Co-Authored-By: Claude Opus 4.6 (1M context) --- .../cli/models/extensions/specification.ts | 28 ++++- packages/app/src/cli/services/context.test.ts | 7 +- .../app/src/cli/services/generate.test.ts | 2 +- .../fetch-extension-specifications.test.ts | 3 - .../fetch-extension-specifications.ts | 38 +++--- .../app/src/cli/utilities/json-schema.test.ts | 111 ++++++------------ packages/app/src/cli/utilities/json-schema.ts | 14 ++- 7 files changed, 90 insertions(+), 113 deletions(-) diff --git a/packages/app/src/cli/models/extensions/specification.ts b/packages/app/src/cli/models/extensions/specification.ts index a91669338aa..6fd5d20f6b1 100644 --- a/packages/app/src/cli/models/extensions/specification.ts +++ b/packages/app/src/cli/models/extensions/specification.ts @@ -1,7 +1,7 @@ import {ZodSchemaType, BaseConfigType, BaseSchema} from './schemas.js' import {ExtensionInstance} from './extension-instance.js' import {blocks} from '../../constants.js' - +import {FlattenedRemoteSpecification} from '../../api/graphql/extension_specifications.js' import {Flag} from '../../utilities/developer-platform-client.js' import {AppConfiguration} from '../app/app.js' import {loadLocalesConfig} from '../../utilities/extensions/locales-configuration.js' @@ -166,6 +166,29 @@ export class ExtensionSpecification { + this.registrationLimit = remoteSpec.registrationLimit + this.externalIdentifier = remoteSpec.externalIdentifier + this.externalName = remoteSpec.externalName + this.experience = remoteSpec.experience as ExtensionExperience + if (remoteSpec.surface) { + this.surface = remoteSpec.surface + } + // WORKAROUND: The value from the API is wrong for this extension + if (remoteSpec.identifier === 'checkout_post_purchase') { + this.surface = 'post_purchase' + } + this.loadedRemoteSpecs = true + return this as RemoteAwareExtensionSpecification + } + + markAsRemoteLoaded(): RemoteAwareExtensionSpecification { + this.loadedRemoteSpecs = true + return this as RemoteAwareExtensionSpecification + } } /** @@ -241,12 +264,13 @@ export function createConfigExtensionSpecification( - spec: Pick, 'identifier' | 'appModuleFeatures' | 'buildConfig'>, + spec: Pick, 'identifier' | 'appModuleFeatures' | 'buildConfig' | 'uidStrategy'>, ) { return createExtensionSpecification({ identifier: spec.identifier, schema: zod.any({}) as unknown as ZodSchemaType, appModuleFeatures: spec.appModuleFeatures, + uidStrategy: spec.uidStrategy, buildConfig: spec.buildConfig ?? {mode: 'none'}, deployConfig: async (config, directory) => { let parsedConfig = configWithoutFirstClassFields(config) diff --git a/packages/app/src/cli/services/context.test.ts b/packages/app/src/cli/services/context.test.ts index 5e3a9c9ae06..4e98324077b 100644 --- a/packages/app/src/cli/services/context.test.ts +++ b/packages/app/src/cli/services/context.test.ts @@ -33,7 +33,7 @@ import { DeveloperPlatformClient, selectDeveloperPlatformClient, } from '../utilities/developer-platform-client.js' -import {RemoteAwareExtensionSpecification} from '../models/extensions/specification.js' + import {TomlFile} from '@shopify/cli-kit/node/toml/toml-file' import {isServiceAccount, isUserAccount} from '@shopify/cli-kit/node/session' import {afterEach, beforeAll, beforeEach, describe, expect, test, vi} from 'vitest' @@ -146,10 +146,7 @@ const mockTomlFileRemove = vi.fn() beforeAll(async () => { const localSpecs = await loadSpecifications.loadLocalExtensionsSpecifications() - const mockedRemoteSpecs = localSpecs.map((spec) => ({ - ...spec, - loadedRemoteSpecs: true, - })) as RemoteAwareExtensionSpecification[] + const mockedRemoteSpecs = localSpecs.map((spec) => spec.markAsRemoteLoaded()) vi.mocked(fetchSpecifications).mockResolvedValue(mockedRemoteSpecs) }) diff --git a/packages/app/src/cli/services/generate.test.ts b/packages/app/src/cli/services/generate.test.ts index b84a5dfd548..2a504f3d156 100644 --- a/packages/app/src/cli/services/generate.test.ts +++ b/packages/app/src/cli/services/generate.test.ts @@ -51,7 +51,7 @@ describe('generate', () => { beforeEach(async () => { const allSpecs = await loadLocalExtensionsSpecifications() - specifications = allSpecs.map((spec) => spec as RemoteAwareExtensionSpecification) + specifications = allSpecs.map((spec) => spec.markAsRemoteLoaded()) developerPlatformClient = testDeveloperPlatformClient() }) diff --git a/packages/app/src/cli/services/generate/fetch-extension-specifications.test.ts b/packages/app/src/cli/services/generate/fetch-extension-specifications.test.ts index 3b81982d2d2..4ce5686807d 100644 --- a/packages/app/src/cli/services/generate/fetch-extension-specifications.test.ts +++ b/packages/app/src/cli/services/generate/fetch-extension-specifications.test.ts @@ -50,7 +50,6 @@ describe('fetchExtensionSpecifications', () => { expect(got).toEqual( expect.arrayContaining([ expect.objectContaining({ - name: 'Product Subscription', externalName: 'Subscription UI', identifier: 'product_subscription', externalIdentifier: 'product_subscription_external', @@ -63,12 +62,10 @@ describe('fetchExtensionSpecifications', () => { expect(got).toEqual( expect.arrayContaining([ expect.objectContaining({ - name: 'Online Store - App Theme Extension', externalName: 'Theme App Extension', identifier: 'theme', externalIdentifier: 'theme_external', registrationLimit: 1, - surface: undefined, }), ]), ) diff --git a/packages/app/src/cli/services/generate/fetch-extension-specifications.ts b/packages/app/src/cli/services/generate/fetch-extension-specifications.ts index 37544a1c672..eddf1c79d0c 100644 --- a/packages/app/src/cli/services/generate/fetch-extension-specifications.ts +++ b/packages/app/src/cli/services/generate/fetch-extension-specifications.ts @@ -35,25 +35,22 @@ export async function fetchSpecifications({ }: FetchSpecificationsOptions): Promise { const result: RemoteSpecification[] = await developerPlatformClient.specifications(app) - const extensionSpecifications: FlattenedRemoteSpecification[] = result + const remoteSpecifications: FlattenedRemoteSpecification[] = result .filter((specification) => ['extension', 'configuration'].includes(specification.experience)) .map((spec) => { - const newSpec = spec as FlattenedRemoteSpecification // WORKAROUND: The identifiers in the API are different for these extensions to the ones the CLI // has been using so far. This is a workaround to keep the CLI working until the API is updated. if (spec.identifier === 'theme_app_extension') spec.identifier = 'theme' if (spec.identifier === 'subscription_management') spec.identifier = 'product_subscription' - newSpec.registrationLimit = spec.options.registrationLimit - newSpec.surface = spec.features?.argo?.surface - - // Hardcoded value for the post purchase extension because the value is wrong in the API - if (spec.identifier === 'checkout_post_purchase') newSpec.surface = 'post_purchase' - - return newSpec + return { + ...spec, + registrationLimit: spec.options.registrationLimit, + surface: spec.features?.argo?.surface, + } }) const local = await loadLocalExtensionsSpecifications() - const updatedSpecs = await mergeLocalAndRemoteSpecs(local, extensionSpecifications) + const updatedSpecs = await mergeLocalAndRemoteSpecs(local, remoteSpecifications) return [...updatedSpecs] } @@ -61,8 +58,6 @@ async function mergeLocalAndRemoteSpecs( local: ExtensionSpecification[], remote: FlattenedRemoteSpecification[], ): Promise { - // Iterate over the remote specs and merge them with the local ones - // If the local spec is missing, and the remote one has a validation schema, create a new local spec using contracts const updated = remote.map(async (remoteSpec) => { let localSpec = local.find((local) => local.identifier === remoteSpec.identifier) if (!localSpec && remoteSpec.validationSchema?.jsonSchema) { @@ -71,17 +66,15 @@ async function mergeLocalAndRemoteSpecs( localSpec = createContractBasedModuleSpecification({ identifier: remoteSpec.identifier, appModuleFeatures: () => (hasLocalization ? ['localization'] : []), + uidStrategy: remoteSpec.options.uidIsClientProvided ? 'uuid' : 'single', }) - localSpec.uidStrategy = remoteSpec.options.uidIsClientProvided ? 'uuid' : 'single' } if (!localSpec) return undefined - const merged = {...localSpec, ...remoteSpec, loadedRemoteSpecs: true} as RemoteAwareExtensionSpecification & - FlattenedRemoteSpecification + const remoteAwareSpec = localSpec.applyRemoteSpecification(remoteSpec) - // If configuration is inside an app.toml -- i.e. single UID mode -- we must be able to parse a partial slice. let handleInvalidAdditionalProperties: HandleInvalidAdditionalProperties - switch (merged.uidStrategy) { + switch (remoteAwareSpec.uidStrategy) { case 'uuid': handleInvalidAdditionalProperties = 'fail' break @@ -93,12 +86,13 @@ async function mergeLocalAndRemoteSpecs( break } - const parseConfigurationObject = await unifiedConfigurationParserFactory(merged, handleInvalidAdditionalProperties) + remoteAwareSpec.parseConfigurationObject = await unifiedConfigurationParserFactory( + remoteAwareSpec, + remoteSpec, + handleInvalidAdditionalProperties, + ) - return { - ...merged, - parseConfigurationObject, - } + return remoteAwareSpec }) const result = getArrayRejectingUndefined(await Promise.all(updated)) diff --git a/packages/app/src/cli/utilities/json-schema.test.ts b/packages/app/src/cli/utilities/json-schema.test.ts index 32dace36518..2b97640ad06 100644 --- a/packages/app/src/cli/utilities/json-schema.test.ts +++ b/packages/app/src/cli/utilities/json-schema.test.ts @@ -1,4 +1,6 @@ import {unifiedConfigurationParserFactory} from './json-schema.js' +import {ExtensionSpecification} from '../models/extensions/specification.js' +import {FlattenedRemoteSpecification} from '../api/graphql/extension_specifications.js' import {describe, test, expect} from 'vitest' import {randomUUID} from '@shopify/cli-kit/node/crypto' @@ -18,19 +20,23 @@ describe('unifiedConfigurationParserFactory', () => { } } - test('falls back to zod parser when no JSON schema is provided', async () => { - // Given - const merged = { + function buildTestInputs(validationSchema?: {jsonSchema: string} | undefined | null) { + const spec = { identifier: randomUUID(), parseConfigurationObject: mockParseConfigurationObject, - validationSchema: undefined, - } + } as unknown as ExtensionSpecification & {loadedRemoteSpecs: true} + const remoteSpec = { + validationSchema, + } as unknown as FlattenedRemoteSpecification + return {spec, remoteSpec} + } - // When - const parser = await unifiedConfigurationParserFactory(merged as any) + test('falls back to zod parser when no JSON schema is provided', async () => { + const {spec, remoteSpec} = buildTestInputs(undefined) + + const parser = await unifiedConfigurationParserFactory(spec, remoteSpec) const result = parser({type: 'product_subscription'}) - // Then expect(result).toEqual({ state: 'ok', data: {type: 'product_subscription'}, @@ -39,20 +45,11 @@ describe('unifiedConfigurationParserFactory', () => { }) test('falls back to zod parser when JSON schema is empty', async () => { - // Given - const merged = { - identifier: randomUUID(), - parseConfigurationObject: mockParseConfigurationObject, - validationSchema: { - jsonSchema: '{}', - }, - } + const {spec, remoteSpec} = buildTestInputs({jsonSchema: '{}'}) - // When - const parser = await unifiedConfigurationParserFactory(merged as any) + const parser = await unifiedConfigurationParserFactory(spec, remoteSpec) const result = parser({type: 'product_subscription'}) - // Then expect(result).toEqual({ state: 'ok', data: {type: 'product_subscription'}, @@ -61,20 +58,13 @@ describe('unifiedConfigurationParserFactory', () => { }) test('validates with both zod and JSON schema when both succeed', async () => { - // Given - const merged = { - identifier: randomUUID(), - parseConfigurationObject: mockParseConfigurationObject, - validationSchema: { - jsonSchema: '{"type":"object","properties":{"type":{"type":"string"}}}', - }, - } + const {spec, remoteSpec} = buildTestInputs({ + jsonSchema: '{"type":"object","properties":{"type":{"type":"string"}}}', + }) - // When - const parser = await unifiedConfigurationParserFactory(merged as any) + const parser = await unifiedConfigurationParserFactory(spec, remoteSpec) const result = parser({type: 'product_subscription'}) - // Then expect(result).toEqual({ state: 'ok', data: {type: 'product_subscription'}, @@ -83,20 +73,13 @@ describe('unifiedConfigurationParserFactory', () => { }) test('returns errors when zod validation fails', async () => { - // Given - const merged = { - identifier: randomUUID(), - parseConfigurationObject: mockParseConfigurationObject, - validationSchema: { - jsonSchema: '{"type":"object","properties":{"type":{"type":"string"}}}', - }, - } + const {spec, remoteSpec} = buildTestInputs({ + jsonSchema: '{"type":"object","properties":{"type":{"type":"string"}}}', + }) - // When - const parser = await unifiedConfigurationParserFactory(merged as any) + const parser = await unifiedConfigurationParserFactory(spec, remoteSpec) const result = parser({type: 'invalid'}) - // Then expect(result.state).toBe('error') expect(result.data).toBeUndefined() expect(result.errors).toHaveLength(1) @@ -104,20 +87,13 @@ describe('unifiedConfigurationParserFactory', () => { }) test('returns errors when JSON schema validation fails', async () => { - // Given - const merged = { - identifier: randomUUID(), - parseConfigurationObject: mockParseConfigurationObject, - validationSchema: { - jsonSchema: '{"type":"object","properties":{"type":{"type":"string"}},"required":["price"]}', - }, - } + const {spec, remoteSpec} = buildTestInputs({ + jsonSchema: '{"type":"object","properties":{"type":{"type":"string"}},"required":["price"]}', + }) - // When - const parser = await unifiedConfigurationParserFactory(merged as any) + const parser = await unifiedConfigurationParserFactory(spec, remoteSpec) const result = parser({type: 'product_subscription'}) - // Then expect(result.state).toBe('error') expect(result.data).toBeUndefined() expect(result.errors).toBeDefined() @@ -126,20 +102,13 @@ describe('unifiedConfigurationParserFactory', () => { }) test('combines errors from both validations', async () => { - // Given - const merged = { - identifier: randomUUID(), - parseConfigurationObject: mockParseConfigurationObject, - validationSchema: { - jsonSchema: '{"type":"object","properties":{"type":{"type":"string"}},"required":["price"]}', - }, - } + const {spec, remoteSpec} = buildTestInputs({ + jsonSchema: '{"type":"object","properties":{"type":{"type":"string"}},"required":["price"]}', + }) - // When - const parser = await unifiedConfigurationParserFactory(merged as any) + const parser = await unifiedConfigurationParserFactory(spec, remoteSpec) const result = parser({type: 'invalid'}) - // Then expect(result.state).toBe('error') expect(result.data).toBeUndefined() expect(result.errors).toBeDefined() @@ -153,19 +122,13 @@ describe('unifiedConfigurationParserFactory', () => { }) test('adds base properties to the JSON schema', async () => { - // Given - const merged = { - identifier: randomUUID(), - parseConfigurationObject: mockParseConfigurationObject, - validationSchema: { - jsonSchema: '{"type":"object","properties":{"custom":{"type":"string"}}}', - }, - } + const {spec, remoteSpec} = buildTestInputs({ + jsonSchema: '{"type":"object","properties":{"custom":{"type":"string"}}}', + }) - // When - const parser = await unifiedConfigurationParserFactory(merged as any) + const parser = await unifiedConfigurationParserFactory(spec, remoteSpec) - // Then - base properties should be accepted + // base properties should be accepted const result = parser({ type: 'product_subscription', handle: 'test-handle', diff --git a/packages/app/src/cli/utilities/json-schema.ts b/packages/app/src/cli/utilities/json-schema.ts index 55855eddd4a..08b1eb9bed9 100644 --- a/packages/app/src/cli/utilities/json-schema.ts +++ b/packages/app/src/cli/utilities/json-schema.ts @@ -28,24 +28,26 @@ const JsonSchemaBaseProperties = { /** * Factory returning a function that can parse a configuration object against a locally defined zod schema, and a remotely defined JSON schema based contract - * @param merged - The merged specification object from the remote and local sources + * @param spec - The local extension specification + * @param remoteSpec - The remote specification containing the validation schema * @returns A function that can parse a configuration object */ export async function unifiedConfigurationParserFactory( - merged: RemoteAwareExtensionSpecification & FlattenedRemoteSpecification, + spec: RemoteAwareExtensionSpecification, + remoteSpec: FlattenedRemoteSpecification, handleInvalidAdditionalProperties: HandleInvalidAdditionalProperties = 'strip', ) { - const contractJsonSchema = merged.validationSchema?.jsonSchema + const contractJsonSchema = remoteSpec.validationSchema?.jsonSchema if (contractJsonSchema === undefined || isEmpty(JSON.parse(contractJsonSchema))) { - return merged.parseConfigurationObject + return spec.parseConfigurationObject } const contract = await normaliseJsonSchema(contractJsonSchema) contract.properties = {...JsonSchemaBaseProperties, ...contract.properties} - const extensionIdentifier = merged.identifier + const extensionIdentifier = spec.identifier const parseConfigurationObject = (config: object): ParseConfigurationResult => { // First we parse with zod. This may also change the format of the data. - const zodParse = merged.parseConfigurationObject(config) + const zodParse = spec.parseConfigurationObject(config) // Then, even if this failed, we try to validate against the contract. const zodValidatedData = zodParse.state === 'ok' ? zodParse.data : undefined