From 59773e9e3737f6c5cc62c55fff456d4bad3cade2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Thu, 17 Oct 2024 17:25:58 +0200 Subject: [PATCH 01/15] use appContext and storeContext instead of devContext --- packages/app/src/cli/commands/app/dev.ts | 21 +++++++ packages/app/src/cli/services/dev.ts | 71 +++++++++++------------- 2 files changed, 54 insertions(+), 38 deletions(-) diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index eaa06954b8e..8b6eceddb8f 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -3,6 +3,8 @@ import {dev, DevOptions} from '../../services/dev.js' import {showApiKeyDeprecationWarning} from '../../prompts/deprecation-warnings.js' import {checkFolderIsValidApp} from '../../models/app/loader.js' import AppCommand, {AppCommandOutput} from '../../utilities/app-command.js' +import {linkedAppContext} from '../../services/app-context.js' +import {storeContext} from '../../services/store-context.js' import {Flags} from '@oclif/core' import {normalizeStoreFqdn} from '@shopify/cli-kit/node/context/fqdn' import {globalFlags} from '@shopify/cli-kit/node/cli' @@ -162,7 +164,26 @@ If you're using the PHP or Ruby app template, then you need to complete the foll await checkFolderIsValidApp(flags.path) + const {app, remoteApp, developerPlatformClient, organization} = await linkedAppContext({ + directory: flags.path, + clientId: apiKey, + forceRelink: flags.reset, + userProvidedConfigName: flags.config, + }) + + const store = await storeContext({ + app, + organization, + developerPlatformClient, + storeFqdn: flags.store, + }) + const devOptions: DevOptions = { + app, + remoteApp, + organization, + developerPlatformClient, + store, directory: flags.path, configName: flags.config, apiKey, diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index 9b5bae45eb3..03380203b90 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -8,13 +8,7 @@ import { startTunnelPlugin, updateURLs, } from './dev/urls.js' -import { - ensureDevContext, - enableDeveloperPreview, - disableDeveloperPreview, - developerPreviewUpdate, - DevContextOptions, -} from './context.js' +import {enableDeveloperPreview, disableDeveloperPreview, developerPreviewUpdate} from './context.js' import {fetchAppPreviewMode} from './dev/fetch.js' import {installAppDependencies} from './dependencies.js' import {DevConfig, DevProcesses, setupDevProcesses} from './dev/processes/setup-dev-processes.js' @@ -22,16 +16,15 @@ import {frontAndBackendConfig} from './dev/processes/utils.js' import {outputUpdateURLsResult, renderDev} from './dev/ui.js' import {DeveloperPreviewController} from './dev/ui/components/Dev.js' import {DevProcessFunction} from './dev/processes/types.js' -import {setCachedAppInfo} from './local-storage.js' +import {getCachedAppInfo, setCachedAppInfo} from './local-storage.js' import {canEnablePreviewMode} from './extensions/common.js' -import {DeveloperPlatformClient, selectDeveloperPlatformClient} from '../utilities/developer-platform-client.js' -import {Web, isCurrentAppSchema, getAppScopesArray, AppInterface} from '../models/app/app.js' -import {OrganizationApp} from '../models/organization.js' +import {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' +import {Web, isCurrentAppSchema, getAppScopesArray, AppInterface, AppLinkedInterface} from '../models/app/app.js' +import {Organization, OrganizationApp, OrganizationStore} from '../models/organization.js' import {getAnalyticsTunnelType} from '../utilities/analytics.js' import {ports} from '../constants.js' import metadata from '../metadata.js' import {AppConfigurationUsedByCli} from '../models/extensions/specifications/types/app_config.js' -import {loadAppConfiguration} from '../models/app/loader.js' import {Config} from '@oclif/core' import {performActionWithRetryAfterRecovery} from '@shopify/cli-kit/common/retry' import {AbortController} from '@shopify/cli-kit/node/abort' @@ -46,6 +39,11 @@ import {hashString} from '@shopify/cli-kit/node/crypto' import {AbortError} from '@shopify/cli-kit/node/error' export interface DevOptions { + app: AppLinkedInterface + remoteApp: OrganizationApp + organization: Organization + developerPlatformClient: DeveloperPlatformClient + store: OrganizationStore directory: string id?: number configName?: string @@ -76,6 +74,8 @@ export async function dev(commandOptions: DevOptions) { } async function prepareForDev(commandOptions: DevOptions): Promise { + const {app, remoteApp, developerPlatformClient, store} = commandOptions + // Be optimistic about tunnel creation and do it as early as possible const tunnelPort = await getAvailableTCPPort() let tunnelClient: TunnelClient | undefined @@ -83,28 +83,19 @@ async function prepareForDev(commandOptions: DevOptions): Promise { tunnelClient = await startTunnelPlugin(commandOptions.commandConfig, tunnelPort, 'cloudflare') } - const {configuration} = await loadAppConfiguration({ - ...commandOptions, - userProvidedConfigName: commandOptions.configName, - }) - let developerPlatformClient = selectDeveloperPlatformClient({configuration}) - const devContextOptions: DevContextOptions = {...commandOptions, developerPlatformClient} - - const { - storeFqdn, - storeId, - remoteApp, - remoteAppUpdated, - updateURLs: cachedUpdateURLs, - localApp: app, - } = await ensureDevContext(devContextOptions) + // const { + // storeFqdn, + // storeId, + // remoteApp, + // remoteAppUpdated, + // updateURLs: cachedUpdateURLs, + // localApp: app, + // } = await ensureDevContext(devContextOptions) - developerPlatformClient = remoteApp.developerPlatformClient ?? developerPlatformClient const apiKey = remoteApp.apiKey - let localApp = app - if (!commandOptions.skipDependenciesInstallation && !localApp.usesWorkspaces) { - localApp = await installAppDependencies(localApp) + if (!commandOptions.skipDependenciesInstallation && !app.usesWorkspaces) { + await installAppDependencies(app) } const graphiqlPort = commandOptions.graphiqlPort || (await getAvailableTCPPort(ports.graphiql)) @@ -126,7 +117,7 @@ async function prepareForDev(commandOptions: DevOptions): Promise { } const {webs, ...network} = await setupNetworkingOptions( - localApp.webs, + app.webs, graphiqlPort, { noTunnel: commandOptions.noTunnel, @@ -135,13 +126,17 @@ async function prepareForDev(commandOptions: DevOptions): Promise { tunnelClient, remoteApp.configuration, ) - localApp.webs = webs + app.webs = webs + + const cachedUpdateURLs = app.configuration.build?.automatically_update_urls_on_dev + + const previousAppId = getCachedAppInfo(commandOptions.directory)?.previousAppId const partnerUrlsUpdated = await handleUpdatingOfPartnerUrls( webs, commandOptions.update, network, - localApp, + app, cachedUpdateURLs, remoteApp, apiKey, @@ -149,11 +144,11 @@ async function prepareForDev(commandOptions: DevOptions): Promise { ) return { - storeFqdn, - storeId, + storeFqdn: store.shopDomain, + storeId: store.shopId, remoteApp, - remoteAppUpdated, - localApp, + remoteAppUpdated: remoteApp.apiKey !== previousAppId, + localApp: app, developerPlatformClient, commandOptions, network, From 3684838cc1d73eecbc127f056612f38003c81540 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Thu, 17 Oct 2024 18:14:06 +0200 Subject: [PATCH 02/15] Some changes to dev --- packages/app/src/cli/commands/app/dev.ts | 16 ++----- packages/app/src/cli/commands/app/logs.ts | 6 ++- packages/app/src/cli/services/app-context.ts | 5 ++- packages/app/src/cli/services/context.ts | 8 ++-- packages/app/src/cli/services/dev.ts | 43 +++++++++++-------- .../app/src/cli/services/store-context.ts | 14 ++++-- 6 files changed, 51 insertions(+), 41 deletions(-) diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index 8b6eceddb8f..042e27c6d82 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -164,7 +164,7 @@ If you're using the PHP or Ruby app template, then you need to complete the foll await checkFolderIsValidApp(flags.path) - const {app, remoteApp, developerPlatformClient, organization} = await linkedAppContext({ + const appContextResult = await linkedAppContext({ directory: flags.path, clientId: apiKey, forceRelink: flags.reset, @@ -172,23 +172,13 @@ If you're using the PHP or Ruby app template, then you need to complete the foll }) const store = await storeContext({ - app, - organization, - developerPlatformClient, - storeFqdn: flags.store, + appContextResult, }) const devOptions: DevOptions = { - app, - remoteApp, - organization, - developerPlatformClient, + ...appContextResult, store, directory: flags.path, - configName: flags.config, - apiKey, - storeFqdn: flags.store, - reset: flags.reset, update: !flags['no-update'], skipDependenciesInstallation: flags['skip-dependencies-installation'], commandConfig, diff --git a/packages/app/src/cli/commands/app/logs.ts b/packages/app/src/cli/commands/app/logs.ts index 75288f0956e..0e089f6d028 100644 --- a/packages/app/src/cli/commands/app/logs.ts +++ b/packages/app/src/cli/commands/app/logs.ts @@ -67,7 +67,11 @@ export default class Logs extends AppCommand { userProvidedConfigName: flags.config, }) - const primaryStore = await storeContext({appContextResult, storeFqdn: flags.store?.[0]}) + const primaryStore = await storeContext({ + appContextResult, + storeFqdn: flags.store?.[0], + forceReselectStore: flags.reset, + }) const logOptions = { ...appContextResult, diff --git a/packages/app/src/cli/services/app-context.ts b/packages/app/src/cli/services/app-context.ts index e839d736d65..47b0dc08741 100644 --- a/packages/app/src/cli/services/app-context.ts +++ b/packages/app/src/cli/services/app-context.ts @@ -98,14 +98,15 @@ export async function linkedAppContext({ setCachedAppInfo({appId: remoteApp.apiKey, title: remoteApp.title, directory, orgId: remoteApp.organizationId}) } - await logMetadata(remoteApp) + await logMetadata(remoteApp, forceRelink) return {app: localApp, remoteApp, developerPlatformClient, specifications, organization} } -async function logMetadata(app: {organizationId: string; apiKey: string}) { +async function logMetadata(app: {organizationId: string; apiKey: string}, resetUsed: boolean) { await metadata.addPublicMetadata(() => ({ partner_id: tryParseInt(app.organizationId), api_key: app.apiKey, + cmd_app_reset_used: resetUsed, })) } diff --git a/packages/app/src/cli/services/context.ts b/packages/app/src/cli/services/context.ts index 6dafe603f26..f0c4aedfac3 100644 --- a/packages/app/src/cli/services/context.ts +++ b/packages/app/src/cli/services/context.ts @@ -177,7 +177,7 @@ export async function ensureDevContext(options: DevContextOptions): Promise { interface ReusedValuesOptions { organization: Organization - selectedApp: OrganizationApp + appName: string selectedStore: OrganizationStore cachedInfo?: CachedAppInfo } @@ -639,7 +639,7 @@ interface ReusedValuesOptions { /** * Message shown to the user in case we are reusing a previous configuration */ -function showReusedDevValues({organization, selectedApp, selectedStore, cachedInfo}: ReusedValuesOptions) { +export function showReusedDevValues({organization, appName, selectedStore, cachedInfo}: ReusedValuesOptions) { if (!cachedInfo) return if (sniffForJson()) return @@ -648,7 +648,7 @@ function showReusedDevValues({organization, selectedApp, selectedStore, cachedIn renderCurrentlyUsedConfigInfo({ org: organization.businessName, - appName: selectedApp.title, + appName, devStore: selectedStore.shopDomain, updateURLs, configFile: cachedInfo.configFile, diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index 03380203b90..96b1a7a1359 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -8,7 +8,12 @@ import { startTunnelPlugin, updateURLs, } from './dev/urls.js' -import {enableDeveloperPreview, disableDeveloperPreview, developerPreviewUpdate} from './context.js' +import { + enableDeveloperPreview, + disableDeveloperPreview, + developerPreviewUpdate, + showReusedDevValues, +} from './context.js' import {fetchAppPreviewMode} from './dev/fetch.js' import {installAppDependencies} from './dependencies.js' import {DevConfig, DevProcesses, setupDevProcesses} from './dev/processes/setup-dev-processes.js' @@ -18,6 +23,7 @@ import {DeveloperPreviewController} from './dev/ui/components/Dev.js' import {DevProcessFunction} from './dev/processes/types.js' import {getCachedAppInfo, setCachedAppInfo} from './local-storage.js' import {canEnablePreviewMode} from './extensions/common.js' +import {writeAppConfigurationFile} from './app/write-app-configuration-file.js' import {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' import {Web, isCurrentAppSchema, getAppScopesArray, AppInterface, AppLinkedInterface} from '../models/app/app.js' import {Organization, OrganizationApp, OrganizationStore} from '../models/organization.js' @@ -45,11 +51,6 @@ export interface DevOptions { developerPlatformClient: DeveloperPlatformClient store: OrganizationStore directory: string - id?: number - configName?: string - apiKey?: string - storeFqdn?: string - reset: boolean update: boolean commandConfig: Config skipDependenciesInstallation: boolean @@ -83,16 +84,25 @@ async function prepareForDev(commandOptions: DevOptions): Promise { tunnelClient = await startTunnelPlugin(commandOptions.commandConfig, tunnelPort, 'cloudflare') } - // const { - // storeFqdn, - // storeId, - // remoteApp, - // remoteAppUpdated, - // updateURLs: cachedUpdateURLs, - // localApp: app, - // } = await ensureDevContext(devContextOptions) + showReusedDevValues({ + appName: remoteApp.title, + selectedStore: store, + cachedInfo: getCachedAppInfo(commandOptions.directory), + organization: commandOptions.organization, + }) - const apiKey = remoteApp.apiKey + // Update the dev_store_url in the app configuration if it doesn't match the store domain + if (app.configuration.build?.dev_store_url !== store.shopDomain) { + const newConfiguration = { + ...app.configuration, + build: { + ...app.configuration.build, + dev_store_url: store.shopDomain, + }, + } + app.configuration = newConfiguration + await writeAppConfigurationFile(newConfiguration, app.configSchema) + } if (!commandOptions.skipDependenciesInstallation && !app.usesWorkspaces) { await installAppDependencies(app) @@ -129,8 +139,8 @@ async function prepareForDev(commandOptions: DevOptions): Promise { app.webs = webs const cachedUpdateURLs = app.configuration.build?.automatically_update_urls_on_dev - const previousAppId = getCachedAppInfo(commandOptions.directory)?.previousAppId + const apiKey = remoteApp.apiKey const partnerUrlsUpdated = await handleUpdatingOfPartnerUrls( webs, @@ -422,7 +432,6 @@ async function logMetadataForDev(options: { cmd_dev_urls_updated: options.shouldUpdateURLs, store_fqdn_hash: hashString(options.storeFqdn), cmd_app_dependency_installation_skipped: options.devOptions.skipDependenciesInstallation, - cmd_app_reset_used: options.devOptions.reset, })) await metadata.addSensitiveMetadata(() => ({ diff --git a/packages/app/src/cli/services/store-context.ts b/packages/app/src/cli/services/store-context.ts index 0783d370441..6faf615ea3e 100644 --- a/packages/app/src/cli/services/store-context.ts +++ b/packages/app/src/cli/services/store-context.ts @@ -13,6 +13,7 @@ import {OrganizationStore} from '../models/organization.js' */ interface StoreContextOptions { appContextResult: LoadedAppContextOutput + forceReselectStore: boolean storeFqdn?: string } @@ -20,15 +21,20 @@ interface StoreContextOptions { * Returns a Store based on the provided options. If a store can't be retrieved, it throws an error. * * If a storeFqdn is explicitly provided, it has preference over anything else. - * If not, check if there is a cached storeFqdn in the app toml configuration. - * If not, fetch all stores for the organization and let the user select one. + * If not, check if there is a cached storeFqdn in the app configuration. If forceReselectStore is true, it will be ignored. + * If still don't have a store, fetch all stores for the organization and let the user select one. */ -export async function storeContext({appContextResult, storeFqdn}: StoreContextOptions): Promise { +export async function storeContext({ + appContextResult, + storeFqdn, + forceReselectStore, +}: StoreContextOptions): Promise { const {app, organization, developerPlatformClient} = appContextResult let selectedStore: OrganizationStore // An explicit storeFqdn has preference over anything else. - const storeFqdnToUse = storeFqdn || app.configuration.build?.dev_store_url + const cachedStoreInToml = forceReselectStore ? undefined : app.configuration.build?.dev_store_url + const storeFqdnToUse = storeFqdn || cachedStoreInToml if (storeFqdnToUse) { selectedStore = await fetchStore(organization, storeFqdnToUse, developerPlatformClient) } else { From 2f177f0189647ab2d4041b047ac115520105f94a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 18 Oct 2024 09:37:14 +0200 Subject: [PATCH 03/15] Move some logic to dev --- packages/app/src/cli/commands/app/dev.ts | 1 + packages/app/src/cli/services/dev.ts | 13 ++++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index 042e27c6d82..39a37d5061b 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -173,6 +173,7 @@ If you're using the PHP or Ruby app template, then you need to complete the foll const store = await storeContext({ appContextResult, + forceReselectStore: flags.reset, }) const devOptions: DevOptions = { diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index 96b1a7a1359..4de6cfe4f01 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -24,6 +24,7 @@ import {DevProcessFunction} from './dev/processes/types.js' import {getCachedAppInfo, setCachedAppInfo} from './local-storage.js' import {canEnablePreviewMode} from './extensions/common.js' import {writeAppConfigurationFile} from './app/write-app-configuration-file.js' +import {fetchAppRemoteConfiguration} from './app/select-app.js' import {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' import {Web, isCurrentAppSchema, getAppScopesArray, AppInterface, AppLinkedInterface} from '../models/app/app.js' import {Organization, OrganizationApp, OrganizationStore} from '../models/organization.js' @@ -31,6 +32,7 @@ import {getAnalyticsTunnelType} from '../utilities/analytics.js' import {ports} from '../constants.js' import metadata from '../metadata.js' import {AppConfigurationUsedByCli} from '../models/extensions/specifications/types/app_config.js' +import {RemoteAwareExtensionSpecification} from '../models/extensions/specification.js' import {Config} from '@oclif/core' import {performActionWithRetryAfterRecovery} from '@shopify/cli-kit/common/retry' import {AbortController} from '@shopify/cli-kit/node/abort' @@ -48,6 +50,7 @@ export interface DevOptions { app: AppLinkedInterface remoteApp: OrganizationApp organization: Organization + specifications: RemoteAwareExtensionSpecification[] developerPlatformClient: DeveloperPlatformClient store: OrganizationStore directory: string @@ -75,7 +78,7 @@ export async function dev(commandOptions: DevOptions) { } async function prepareForDev(commandOptions: DevOptions): Promise { - const {app, remoteApp, developerPlatformClient, store} = commandOptions + const {app, remoteApp, developerPlatformClient, store, specifications} = commandOptions // Be optimistic about tunnel creation and do it as early as possible const tunnelPort = await getAvailableTCPPort() @@ -84,6 +87,14 @@ async function prepareForDev(commandOptions: DevOptions): Promise { tunnelClient = await startTunnelPlugin(commandOptions.commandConfig, tunnelPort, 'cloudflare') } + const remoteConfiguration = await fetchAppRemoteConfiguration( + remoteApp, + developerPlatformClient, + specifications, + remoteApp.flags, + ) + remoteApp.configuration = remoteConfiguration + showReusedDevValues({ appName: remoteApp.title, selectedStore: store, From 5b5c0e858d303513d9ef2d26f6db441749669a0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 18 Oct 2024 10:12:19 +0200 Subject: [PATCH 04/15] Update code to show reused values --- packages/app/src/cli/services/context.ts | 27 +++++++++++++---------- packages/app/src/cli/services/dev.ts | 7 +++--- packages/app/src/cli/services/dev/urls.ts | 21 +++++++----------- 3 files changed, 27 insertions(+), 28 deletions(-) diff --git a/packages/app/src/cli/services/context.ts b/packages/app/src/cli/services/context.ts index f0c4aedfac3..cbee867dd5a 100644 --- a/packages/app/src/cli/services/context.ts +++ b/packages/app/src/cli/services/context.ts @@ -16,6 +16,7 @@ import { isCurrentAppSchema, CurrentAppConfiguration, AppCreationDefaultOptions, + AppLinkedInterface, } from '../models/app/app.js' import {Identifiers, UuidOnlyIdentifiers, updateAppIdentifiers, getAppIdentifiers} from '../models/app/identifiers.js' import {Organization, OrganizationApp, OrganizationStore} from '../models/organization.js' @@ -175,14 +176,14 @@ export async function ensureDevContext(options: DevContextOptions): Promise { interface ReusedValuesOptions { organization: Organization - appName: string + app: AppLinkedInterface + remoteApp: OrganizationApp selectedStore: OrganizationStore cachedInfo?: CachedAppInfo } @@ -639,16 +641,17 @@ interface ReusedValuesOptions { /** * Message shown to the user in case we are reusing a previous configuration */ -export function showReusedDevValues({organization, appName, selectedStore, cachedInfo}: ReusedValuesOptions) { +export function showReusedDevValues({organization, app, remoteApp, selectedStore, cachedInfo}: ReusedValuesOptions) { if (!cachedInfo) return if (sniffForJson()) return let updateURLs = 'Not yet configured' - if (cachedInfo.updateURLs !== undefined) updateURLs = cachedInfo.updateURLs ? 'Yes' : 'No' + const updateURLsValue = app.configuration.build?.automatically_update_urls_on_dev + if (updateURLsValue !== undefined) updateURLs = updateURLsValue ? 'Yes' : 'No' renderCurrentlyUsedConfigInfo({ org: organization.businessName, - appName, + appName: remoteApp.title, devStore: selectedStore.shopDomain, updateURLs, configFile: cachedInfo.configFile, diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index 4de6cfe4f01..18a1cbb1638 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -26,7 +26,7 @@ import {canEnablePreviewMode} from './extensions/common.js' import {writeAppConfigurationFile} from './app/write-app-configuration-file.js' import {fetchAppRemoteConfiguration} from './app/select-app.js' import {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' -import {Web, isCurrentAppSchema, getAppScopesArray, AppInterface, AppLinkedInterface} from '../models/app/app.js' +import {Web, isCurrentAppSchema, getAppScopesArray, AppLinkedInterface} from '../models/app/app.js' import {Organization, OrganizationApp, OrganizationStore} from '../models/organization.js' import {getAnalyticsTunnelType} from '../utilities/analytics.js' import {ports} from '../constants.js' @@ -96,7 +96,8 @@ async function prepareForDev(commandOptions: DevOptions): Promise { remoteApp.configuration = remoteConfiguration showReusedDevValues({ - appName: remoteApp.title, + app, + remoteApp, selectedStore: store, cachedInfo: getCachedAppInfo(commandOptions.directory), organization: commandOptions.organization, @@ -254,7 +255,7 @@ async function handleUpdatingOfPartnerUrls( proxyUrl: string currentUrls: PartnersURLs }, - localApp: AppInterface, + localApp: AppLinkedInterface, cachedUpdateURLs: boolean | undefined, remoteApp: Omit & {apiSecret?: string | undefined}, apiKey: string, diff --git a/packages/app/src/cli/services/dev/urls.ts b/packages/app/src/cli/services/dev/urls.ts index b94a3be6ac7..f687adff9d4 100644 --- a/packages/app/src/cli/services/dev/urls.ts +++ b/packages/app/src/cli/services/dev/urls.ts @@ -1,12 +1,11 @@ import {updateURLsPrompt} from '../../prompts/dev.js' import { AppConfigurationInterface, - AppInterface, + AppLinkedInterface, CurrentAppConfiguration, isCurrentAppSchema, } from '../../models/app/app.js' import {UpdateURLsSchema, UpdateURLsVariables} from '../../api/graphql/update_urls.js' -import {setCachedAppInfo} from '../local-storage.js' import {writeAppConfigurationFile} from '../app/write-app-configuration-file.js' import {AppConfigurationUsedByCli} from '../../models/extensions/specifications/types/app_config.js' import {DeveloperPlatformClient} from '../../utilities/developer-platform-client.js' @@ -246,7 +245,7 @@ interface ShouldOrPromptUpdateURLsOptions { appDirectory: string cachedUpdateURLs?: boolean newApp?: boolean - localApp?: AppInterface + localApp: AppLinkedInterface apiKey: string } @@ -261,17 +260,13 @@ export async function shouldOrPromptUpdateURLs(options: ShouldOrPromptUpdateURLs options.currentURLs.redirectUrlWhitelist, ) - if (options.localApp && isCurrentAppSchema(options.localApp.configuration)) { - const localConfiguration = options.localApp.configuration - localConfiguration.build = { - ...localConfiguration.build, - automatically_update_urls_on_dev: shouldUpdateURLs, - } - - await writeAppConfigurationFile(localConfiguration, options.localApp.configSchema) - } else { - setCachedAppInfo({directory: options.appDirectory, updateURLs: shouldUpdateURLs}) + const localConfiguration = options.localApp.configuration + localConfiguration.build = { + ...localConfiguration.build, + automatically_update_urls_on_dev: shouldUpdateURLs, } + + await writeAppConfigurationFile(localConfiguration, options.localApp.configSchema) } return shouldUpdateURLs } From 33e2f6acac00affbc3d3a22bf72594d31200fdef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 18 Oct 2024 11:22:08 +0200 Subject: [PATCH 05/15] Update patch toml function to replace arrays instead of merging --- .../services/app/patch-app-configuration-file.test.ts | 9 ++++++--- .../src/cli/services/app/patch-app-configuration-file.ts | 9 ++++++++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/app/src/cli/services/app/patch-app-configuration-file.test.ts b/packages/app/src/cli/services/app/patch-app-configuration-file.test.ts index 87d0eedabfe..6728734aa87 100644 --- a/packages/app/src/cli/services/app/patch-app-configuration-file.test.ts +++ b/packages/app/src/cli/services/app/patch-app-configuration-file.test.ts @@ -33,7 +33,7 @@ function writeDefaulToml(tmpDir: string) { } describe('patchAppConfigurationFile', () => { - test('updates existing configuration with new values and adds new top-levelfields', async () => { + test('updates existing configuration with new values and adds new top-levelfields, replaces arrays', async () => { await inTemporaryDirectory(async (tmpDir) => { const configPath = writeDefaulToml(tmpDir) const patch = { @@ -42,6 +42,9 @@ describe('patchAppConfigurationFile', () => { access_scopes: { use_legacy_install_flow: false, }, + auth: { + redirect_urls: ['https://example.com/redirect3', 'https://example.com/redirect4'], + }, } await patchAppConfigurationFile({path: configPath, patch, schema}) @@ -61,8 +64,8 @@ use_legacy_install_flow = false [auth] redirect_urls = [ - "https://example.com/redirect", - "https://example.com/redirect2" + "https://example.com/redirect3", + "https://example.com/redirect4" ] [webhooks] diff --git a/packages/app/src/cli/services/app/patch-app-configuration-file.ts b/packages/app/src/cli/services/app/patch-app-configuration-file.ts index 5bb533407bb..8d72eecf143 100644 --- a/packages/app/src/cli/services/app/patch-app-configuration-file.ts +++ b/packages/app/src/cli/services/app/patch-app-configuration-file.ts @@ -22,7 +22,10 @@ export interface PatchTomlOptions { export async function patchAppConfigurationFile({path, patch, schema}: PatchTomlOptions) { const tomlContents = await readFile(path) const configuration = decodeToml(tomlContents) - const updatedConfig = deepMergeObjects(configuration, patch) + + // Deep merge the configuration with the patch. + // Use replaceArrayStrategy to replace the destination array with the source array. (Arrays are not merged) + const updatedConfig = deepMergeObjects(configuration, patch, replaceArrayStrategy) // Re-parse the config with the schema to validate the patch and keep the same order in the file // Make every field optional to not crash on invalid tomls that are missing fields. @@ -33,3 +36,7 @@ export async function patchAppConfigurationFile({path, patch, schema}: PatchToml encodedString = addDefaultCommentsToToml(encodedString) await writeFile(path, encodedString) } + +export function replaceArrayStrategy(_: unknown[], newArray: unknown[]): unknown[] { + return newArray +} From 7ef4c41adb07b2d503eac80a403274561c1f0ee0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 18 Oct 2024 11:32:59 +0200 Subject: [PATCH 06/15] Make store-context call convertTransferDisabledStore --- packages/app/src/cli/services/store-context.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/app/src/cli/services/store-context.ts b/packages/app/src/cli/services/store-context.ts index 164130ff1df..ce33202dcf5 100644 --- a/packages/app/src/cli/services/store-context.ts +++ b/packages/app/src/cli/services/store-context.ts @@ -1,5 +1,5 @@ import {fetchStore} from './dev/fetch.js' -import {selectStore} from './dev/select-store.js' +import {convertToTransferDisabledStoreIfNeeded, selectStore} from './dev/select-store.js' import {LoadedAppContextOutput} from './app-context.js' import {OrganizationStore} from '../models/organization.js' @@ -38,6 +38,8 @@ export async function storeContext({ const storeFqdnToUse = storeFqdn || cachedStoreInToml if (storeFqdnToUse) { selectedStore = await fetchStore(organization, storeFqdnToUse, developerPlatformClient) + // never automatically convert a store provided via the command line + await convertToTransferDisabledStoreIfNeeded(selectedStore, organization.id, developerPlatformClient, 'never') } else { // If no storeFqdn is provided, fetch all stores for the organization and let the user select one. const allStores = await developerPlatformClient.devStoresForOrg(organization.id) From 15b40d0441ea852ec07e8ddd90c639e4d74832f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 18 Oct 2024 11:37:55 +0200 Subject: [PATCH 07/15] Delete ensureDevContext --- packages/app/src/cli/services/context.test.ts | 642 +----------------- packages/app/src/cli/services/context.ts | 315 +-------- 2 files changed, 9 insertions(+), 948 deletions(-) diff --git a/packages/app/src/cli/services/context.test.ts b/packages/app/src/cli/services/context.test.ts index 67be4886a78..28fce15ba7e 100644 --- a/packages/app/src/cli/services/context.test.ts +++ b/packages/app/src/cli/services/context.test.ts @@ -1,10 +1,10 @@ -import {fetchOrganizations, fetchOrgFromId, fetchStoreByDomain} from './dev/fetch.js' +import {fetchOrganizations, fetchOrgFromId} from './dev/fetch.js' import {selectOrCreateApp} from './dev/select-app.js' -import {selectStore, convertToTransferDisabledStoreIfNeeded} from './dev/select-store.js' +import {selectStore} from './dev/select-store.js' import {ensureDeploymentIdsPresence} from './context/identifiers.js' -import {DevContextOptions, ensureDevContext, ensureDeployContext, ensureThemeExtensionDevContext} from './context.js' +import {ensureDeployContext, ensureThemeExtensionDevContext} from './context.js' import {createExtension} from './dev/create-extension.js' -import {CachedAppInfo, clearCachedAppInfo, getCachedAppInfo, setCachedAppInfo} from './local-storage.js' +import {CachedAppInfo} from './local-storage.js' import link from './app/config/link.js' import {fetchSpecifications} from './generate/fetch-extension-specifications.js' import * as patchAppConfigurationFile from './app/patch-app-configuration-file.js' @@ -28,22 +28,14 @@ import { buildVersionedAppSchema, } from '../models/app/app.test-data.js' import metadata from '../metadata.js' -import { - AppConfigurationStateLinked, - getAppConfigurationFileName, - isWebType, - loadApp, - loadAppConfiguration, -} from '../models/app/loader.js' -import {AppInterface, AppLinkedInterface, CurrentAppConfiguration} from '../models/app/app.js' +import {AppConfigurationStateLinked, getAppConfigurationFileName, isWebType, loadApp} from '../models/app/loader.js' +import {AppInterface, AppLinkedInterface} from '../models/app/app.js' import * as loadSpecifications from '../models/extensions/load-specifications.js' import {DeveloperPlatformClient, selectDeveloperPlatformClient} from '../utilities/developer-platform-client.js' import {RemoteAwareExtensionSpecification} from '../models/extensions/specification.js' import {afterEach, beforeAll, beforeEach, describe, expect, test, vi} from 'vitest' import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' import {getPackageManager} from '@shopify/cli-kit/node/node-package-manager' -import {inTemporaryDirectory, readFile, writeFileSync} from '@shopify/cli-kit/node/fs' -import {joinPath} from '@shopify/cli-kit/node/path' import {renderConfirmationPrompt, renderInfo, renderTasks, Task} from '@shopify/cli-kit/node/ui' import {Config} from '@oclif/core' @@ -92,17 +84,6 @@ const STORE2: OrganizationStore = { convertableToPartnerTest: false, } -const devOptions = (options: object = {}): DevContextOptions => { - return { - directory: 'app_directory', - reset: false, - developerPlatformClient: buildDeveloperPlatformClient({ - appFromId: () => Promise.resolve(APP2), - }), - ...options, - } -} - const ORG_AND_APPS_RESPONSE = { organization: ORG1, apps: [APP1, APP2], @@ -221,617 +202,6 @@ afterEach(() => { mockAndCaptureOutput().clear() }) -describe('ensureDevContext', async () => { - beforeEach(async () => { - const {schema: configSchema} = await buildVersionedAppSchema() - vi.mocked(loadAppConfiguration).mockResolvedValue({ - directory: '/app', - configuration: { - path: '/app/shopify.app.toml', - scopes: 'read_products', - }, - configSchema, - specifications: [], - remoteFlags: [], - }) - }) - - test('returns selected data using config file set in cache', async () => { - await inTemporaryDirectory(async (tmp) => { - // Given - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(buildDeveloperPlatformClient()) - vi.mocked(getCachedAppInfo).mockReturnValue(CACHED1_WITH_CONFIG) - const patchAppConfigurationFileSpy = vi - .spyOn(patchAppConfigurationFile, 'patchAppConfigurationFile') - .mockResolvedValue() - vi.mocked(loadAppConfiguration).mockReset() - const {schema: configSchema} = await buildVersionedAppSchema() - const localApp = { - configuration: { - ...DEFAULT_CONFIG, - path: joinPath(tmp, CACHED1_WITH_CONFIG.configFile!), - name: APP2.apiKey, - client_id: APP2.apiKey, - build: { - automatically_update_urls_on_dev: true, - dev_store_url: STORE1.shopDomain, - }, - } as CurrentAppConfiguration, - } - vi.mocked(loadAppConfiguration).mockResolvedValue({ - directory: tmp, - configuration: localApp.configuration, - configSchema, - specifications: [], - remoteFlags: [], - }) - vi.mocked(fetchStoreByDomain).mockResolvedValue({organization: ORG1, store: STORE1}) - const app = await mockApp(tmp, localApp) - vi.mocked(loadApp).mockResolvedValue(app) - const options = devOptions() - - // When - const got = await ensureDevContext(options) - - // Then - expect(got).toEqual({ - remoteApp: {...APP2, apiSecret: 'secret2'}, - storeFqdn: STORE1.shopDomain, - storeId: STORE1.shopId, - remoteAppUpdated: true, - updateURLs: true, - localApp: app, - organization: 'org1', - configFile: 'shopify.app.toml', - }) - expect(setCachedAppInfo).not.toHaveBeenCalled() - - expect(metadata.getAllPublicMetadata()).toMatchObject({ - api_key: APP2.apiKey, - partner_id: 1, - }) - patchAppConfigurationFileSpy.mockRestore() - }) - }) - - test('returns context from client-id flag rather than config in cache', async () => { - await inTemporaryDirectory(async (tmp) => { - // Given - const expectedContent = `# Learn more about configuring your app at https://shopify.dev/docs/apps/tools/cli/configuration -name = "my app" -client_id = "12345" -application_url = "https://myapp.com" -embedded = true - -[access_scopes] -# Learn more at https://shopify.dev/docs/apps/tools/cli/configuration#access_scopes -scopes = "read_products" - -[webhooks] -api_version = "2023-04" - -[build] -dev_store_url = "domain1" -` - const filePath = joinPath(tmp, 'shopify.app.toml') - writeFileSync(filePath, expectedContent) - vi.mocked(getCachedAppInfo).mockReturnValue(CACHED1_WITH_CONFIG) - vi.mocked(loadAppConfiguration).mockReset() - const {schema: configSchema} = await buildVersionedAppSchema() - vi.mocked(loadAppConfiguration).mockResolvedValue({ - directory: tmp, - configuration: testAppWithConfig({ - config: { - path: joinPath(tmp, CACHED1_WITH_CONFIG.configFile!), - name: APP1.apiKey, - client_id: APP1.apiKey, - build: { - automatically_update_urls_on_dev: true, - dev_store_url: STORE1.shopDomain, - }, - }, - }).configuration, - configSchema, - specifications: [], - remoteFlags: [], - }) - vi.mocked(fetchStoreByDomain).mockResolvedValue({organization: ORG1, store: STORE1}) - const options = devOptions({apiKey: APP2.apiKey}) - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) - - // When - const got = await ensureDevContext(options) - - // Then - expect(got).toEqual({ - remoteApp: {...APP2, apiSecret: 'secret2'}, - storeFqdn: STORE1.shopDomain, - storeId: STORE1.shopId, - remoteAppUpdated: true, - updateURLs: true, - organization: 'org1', - configFile: 'shopify.app.toml', - }) - expect(setCachedAppInfo).not.toHaveBeenCalled() - - expect(metadata.getAllPublicMetadata()).toMatchObject({ - api_key: APP2.apiKey, - partner_id: 1, - }) - - const content = await readFile(joinPath(tmp, 'shopify.app.toml')) - expect(content).toEqual(expectedContent) - }) - }) - - test('loads the correct file when config flag is passed in', async () => { - await inTemporaryDirectory(async (tmp) => { - // Given - writeFileSync(joinPath(tmp, 'shopify.app.dev.toml'), '') - vi.mocked(getCachedAppInfo).mockReturnValue(undefined) - vi.mocked(loadAppConfiguration).mockReset() - const localApp = { - configuration: { - path: joinPath(tmp, 'shopify.app.dev.toml'), - name: 'my app', - client_id: APP2.apiKey, - scopes: 'write_products', - webhooks: {api_version: '2023-04'}, - application_url: 'https://myapp.com', - } as CurrentAppConfiguration, - } - const {schema: configSchema} = await buildVersionedAppSchema() - vi.mocked(loadAppConfiguration).mockResolvedValue({ - directory: tmp, - configuration: localApp.configuration, - configSchema, - specifications: [], - remoteFlags: [], - }) - vi.mocked(fetchStoreByDomain).mockResolvedValue({organization: ORG1, store: STORE1}) - const app = await mockApp(tmp, localApp) - vi.mocked(loadApp).mockResolvedValue(app) - const options = devOptions() - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) - - // When - await ensureDevContext(options) - - // Then - expect(loadAppConfiguration).toHaveBeenCalledWith({ - directory: 'app_directory', - }) - }) - }) - - test('prompts to select store when not set in config file', async () => { - await inTemporaryDirectory(async (tmp) => { - // Given - const filePath = joinPath(tmp, 'shopify.app.dev.toml') - const tomlContent = `# Learn more about configuring your app at https://shopify.dev/docs/apps/tools/cli/configuration -client_id = "12345" -` - writeFileSync(filePath, tomlContent) - vi.mocked(loadAppConfiguration).mockReset() - const {schema: configSchema} = await buildVersionedAppSchema() - const localApp = { - configuration: { - ...DEFAULT_CONFIG, - client_id: APP2.apiKey, - path: joinPath(tmp, 'shopify.app.dev.toml'), - } as CurrentAppConfiguration, - } - - vi.mocked(loadAppConfiguration).mockResolvedValue({ - directory: tmp, - configuration: localApp.configuration, - configSchema, - specifications: [], - remoteFlags: [], - }) - const app = await mockApp(tmp, localApp) - vi.mocked(loadApp).mockResolvedValue(app) - const options = devOptions() - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) - - // When - await ensureDevContext(options) - - // Then - expect(selectStore).toHaveBeenCalled() - const content = await readFile(joinPath(tmp, 'shopify.app.dev.toml')) - const expectedContent = `# Learn more about configuring your app at https://shopify.dev/docs/apps/tools/cli/configuration - -client_id = "12345" - -[build] -dev_store_url = "domain1" -` - expect(content).toEqual(expectedContent) - }) - }) - - test('shows the correct banner content when running for the first time with linked config file', async () => { - await inTemporaryDirectory(async (tmp) => { - // Given - writeFileSync(joinPath(tmp, 'shopify.app.toml'), '') - vi.mocked(getCachedAppInfo).mockReturnValue(undefined) - vi.mocked(loadAppConfiguration).mockReset() - const {schema: configSchema} = await buildVersionedAppSchema() - const localApp = { - configuration: { - ...DEFAULT_CONFIG, - client_id: APP2.apiKey, - path: joinPath(tmp, 'shopify.app.toml'), - } as CurrentAppConfiguration, - } - vi.mocked(loadAppConfiguration).mockResolvedValue({ - directory: tmp, - configuration: localApp.configuration, - configSchema, - specifications: [], - remoteFlags: [], - }) - - vi.mocked(getAppConfigurationFileName).mockReturnValue('shopify.app.toml') - vi.mocked(fetchStoreByDomain).mockResolvedValue({organization: ORG1, store: STORE1}) - const app = await mockApp(tmp, localApp) - vi.mocked(loadApp).mockResolvedValue(app) - const options = devOptions() - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) - - // When - await ensureDevContext(options) - - // Then - expect(renderInfo).toHaveBeenCalledWith({ - body: [ - { - list: { - items: [ - `Org: ${ORG1.businessName}`, - `App: ${APP2.title}`, - `Dev store: ${STORE1.shopDomain}`, - 'Update URLs: Not yet configured', - ], - }, - }, - '\n', - 'You can pass ', - { - command: '--reset', - }, - ' to your command to reset your app configuration.', - ], - headline: 'Using shopify.app.toml for default values:', - }) - }) - }) - - test('returns selected data and updates internal state, without cached state', async () => { - // Given - vi.mocked(getCachedAppInfo).mockReturnValue(undefined) - const options = devOptions() - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) - - // When - const got = await ensureDevContext(options) - - // Then - expect(got).toEqual({ - remoteApp: {...APP1, apiSecret: 'secret1'}, - storeFqdn: STORE1.shopDomain, - storeId: STORE1.shopId, - remoteAppUpdated: true, - updateURLs: undefined, - organization: 'org1', - }) - expect(setCachedAppInfo).toHaveBeenNthCalledWith(1, { - appId: APP1.apiKey, - title: APP1.title, - storeFqdn: STORE1.shopDomain, - directory: options.directory, - orgId: ORG1.id, - }) - - expect(metadata.getAllPublicMetadata()).toMatchObject({ - api_key: APP1.apiKey, - partner_id: 1, - }) - }) - - test('returns remoteAppUpdated true when previous app id is different', async () => { - // Given - vi.mocked(getCachedAppInfo).mockReturnValue({...CACHED1_WITH_CONFIG, previousAppId: APP2.apiKey}) - // vi.mocked(fetchOrgFromId).mockResolvedValueOnce(ORG2) - vi.mocked(fetchStoreByDomain).mockResolvedValue({organization: ORG1, store: STORE1}) - - // When - const options = devOptions({ - developerPlatformClient: buildDeveloperPlatformClient({ - appFromId: () => Promise.resolve(APP1), - }), - }) - const got = await ensureDevContext(options) - - // Then - expect(got).toEqual({ - remoteApp: {...APP1, apiSecret: 'secret1'}, - storeFqdn: STORE1.shopDomain, - storeId: STORE1.shopId, - remoteAppUpdated: true, - updateURLs: undefined, - organization: 'org1', - configFile: 'shopify.app.toml', - }) - }) - - test('returns selected data and updates internal state, with cached state', async () => { - // Given - vi.mocked(getCachedAppInfo).mockReturnValue({...CACHED1, previousAppId: APP1.apiKey}) - vi.mocked(fetchStoreByDomain).mockResolvedValue({organization: ORG1, store: STORE1}) - - // When - const options = devOptions({ - developerPlatformClient: buildDeveloperPlatformClient({ - appFromId: () => Promise.resolve(APP1), - orgAndApps: () => Promise.resolve(ORG_AND_APPS_RESPONSE), - }), - }) - const got = await ensureDevContext(options) - - // Then - expect(got).toEqual({ - remoteApp: {...APP1, apiSecret: 'secret1'}, - storeFqdn: STORE1.shopDomain, - storeId: STORE1.shopId, - remoteAppUpdated: false, - updateURLs: undefined, - organization: 'org1', - }) - expect(fetchOrganizations).not.toHaveBeenCalled() - expect(setCachedAppInfo).toHaveBeenNthCalledWith(1, { - appId: APP1.apiKey, - title: APP1.title, - storeFqdn: STORE1.shopDomain, - directory: options.directory, - orgId: ORG1.id, - }) - expect(renderInfo).toHaveBeenCalledWith({ - body: [ - { - list: { - items: [ - 'Org: org1', - 'App: app1', - 'Dev store: domain1', - 'Update URLs: Not yet configured', - ], - }, - }, - '\n', - 'You can pass ', - { - command: '--reset', - }, - ' to your command to reset your app configuration.', - ], - headline: 'Using these settings:', - }) - expect(options.developerPlatformClient.orgAndApps).not.toBeCalled() - }) - - test('suppresses info box when customLogInfoBox flag is passed', async () => { - // Given - vi.mocked(getCachedAppInfo).mockReturnValue({...CACHED1, previousAppId: APP1.apiKey}) - vi.mocked(fetchStoreByDomain).mockResolvedValue({organization: ORG1, store: STORE1}) - - // When - const options = devOptions({ - customInfoBox: true, - storeFqdn: 'domain1', - storeFqdns: ['domain1', 'domain2'], - developerPlatformClient: buildDeveloperPlatformClient({ - appFromId: () => Promise.resolve(APP1), - orgAndApps: () => Promise.resolve(ORG_AND_APPS_RESPONSE), - }), - }) - await ensureDevContext(options) - - // Then - expect(renderInfo).not.toHaveBeenCalled() - }) - - test('returns selected data and updates internal state, with inputs from flags', async () => { - // Given - vi.mocked(getCachedAppInfo).mockReturnValue(undefined) - vi.mocked(convertToTransferDisabledStoreIfNeeded).mockResolvedValueOnce(true) - vi.mocked(fetchStoreByDomain).mockResolvedValue({organization: ORG1, store: STORE1}) - const options = devOptions({ - apiKey: 'key2', - storeFqdn: 'domain1', - developerPlatformClient: buildDeveloperPlatformClient({ - appFromId: () => Promise.resolve(APP2), - orgAndApps: () => Promise.resolve(ORG_AND_APPS_RESPONSE), - }), - }) - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) - - // When - const got = await ensureDevContext(options) - - // Then - expect(got).toEqual({ - remoteApp: {...APP2, apiSecret: 'secret2'}, - storeFqdn: STORE1.shopDomain, - storeId: STORE1.shopId, - remoteAppUpdated: true, - updateURLs: undefined, - organization: 'org1', - }) - expect(setCachedAppInfo).toHaveBeenNthCalledWith(1, { - appId: APP2.apiKey, - directory: options.directory, - storeFqdn: STORE1.shopDomain, - orgId: ORG1.id, - title: APP2.title, - }) - expect(fetchOrganizations).toBeCalled() - expect(selectOrCreateApp).not.toBeCalled() - expect(selectStore).not.toBeCalled() - expect(options.developerPlatformClient.orgAndApps).not.toBeCalled() - }) - - test('throws if the store input is not valid', async () => { - // Given - vi.mocked(getCachedAppInfo).mockReturnValue(undefined) - vi.mocked(fetchStoreByDomain).mockResolvedValue({organization: ORG1, store: undefined}) - const options = devOptions({ - apiKey: 'key1', - storeFqdn: 'invalid_store_domain', - }) - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) - - // When - const got = ensureDevContext(options) - - // Then - await expect(got).rejects.toThrow(/Could not find invalid_store_domain/) - }) - - test('resets cached state if reset is true', async () => { - // Given - vi.mocked(getCachedAppInfo).mockReturnValueOnce(CACHED1) - const options = devOptions({reset: true}) - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) - - // When - await ensureDevContext(options) - - // Then - expect(clearCachedAppInfo).toHaveBeenCalledWith(options.directory) - expect(options.developerPlatformClient.appsForOrg).toBeCalled() - expect(link).toBeCalled() - }) - - test('reset triggers link if opted into config in code', async () => { - await inTemporaryDirectory(async (tmp) => { - // Given - writeFileSync(joinPath(tmp, 'shopify.app.dev.toml'), '') - vi.mocked(getCachedAppInfo).mockReturnValueOnce(CACHED1_WITH_CONFIG) - const filePath = joinPath(tmp, 'shopify.app.dev.toml') - const localApp = { - configuration: { - ...DEFAULT_CONFIG, - path: filePath, - client_id: APP2.apiKey, - name: APP2.apiKey, - application_url: 'https://example.com', - webhooks: {api_version: '2023-04'}, - } as CurrentAppConfiguration, - } - const {schema: configSchema} = await buildVersionedAppSchema() - vi.mocked(loadAppConfiguration).mockResolvedValue({ - directory: tmp, - configuration: localApp.configuration, - configSchema, - specifications: [], - remoteFlags: [], - }) - const app = await mockApp(tmp, localApp) - vi.mocked(loadApp).mockResolvedValue(app) - const options = devOptions({reset: true}) - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) - - // When - const got = await ensureDevContext(options) - - // Then - expect(link).toBeCalled() - expect(got.remoteApp).toEqual({...APP2, apiSecret: 'secret2'}) - }) - }) - - test('links an app when running dev for the first time', async () => { - // Given - const options = devOptions() - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) - - // When - await ensureDevContext(options) - - // Then - expect(link).toBeCalled() - }) - - describe('when --json is in argv', () => { - let originalArgv: string[] - - beforeEach(() => { - originalArgv = process.argv - }) - - // Restore the original process.argv - afterEach(() => { - process.argv = originalArgv - }) - - test('Does not display used dev values when using json output', async () => { - vi.mocked(getCachedAppInfo).mockReturnValue({...CACHED1, previousAppId: APP1.apiKey}) - vi.mocked(fetchStoreByDomain).mockResolvedValue({organization: ORG1, store: STORE1}) - - // When - const options = devOptions({ - developerPlatformClient: buildDeveloperPlatformClient({ - appFromId: () => Promise.resolve(APP1), - }), - }) - process.argv = ['', '', '--json'] - await ensureDevContext(options) - - expect(renderInfo).not.toBeCalled() - }) - }) - - test('links app if no app configs exist & cache has a current config file defined', async () => { - await inTemporaryDirectory(async (tmp) => { - // Given - writeFileSync(joinPath(tmp, 'shopify.app.toml'), '') - vi.mocked(getCachedAppInfo).mockReturnValueOnce(CACHED1_WITH_CONFIG) - const filePath = joinPath(tmp, 'shopify.app.toml') - const {schema: configSchema} = await buildVersionedAppSchema() - const localApp = { - configuration: { - ...DEFAULT_CONFIG, - path: filePath, - client_id: APP2.apiKey, - name: APP2.apiKey, - application_url: 'https://example.com', - webhooks: {api_version: '2023-04'}, - } as CurrentAppConfiguration, - } - vi.mocked(loadAppConfiguration).mockResolvedValue({ - directory: tmp, - configuration: localApp.configuration, - configSchema, - specifications: [], - remoteFlags: [], - }) - const app = await mockApp(tmp, localApp) - vi.mocked(loadApp).mockResolvedValue(app) - const options = devOptions() - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) - - // When - const got = await ensureDevContext(options) - - // Then - expect(link).toBeCalled() - expect(got.remoteApp).toEqual({...APP2, apiSecret: 'secret2'}) - }) - }) -}) - describe('ensureDeployContext', () => { test('prompts the user to include the configuration and persist the flag if the flag is not present', async () => { // Given diff --git a/packages/app/src/cli/services/context.ts b/packages/app/src/cli/services/context.ts index e5a1487bf42..94eed1c296c 100644 --- a/packages/app/src/cli/services/context.ts +++ b/packages/app/src/cli/services/context.ts @@ -1,18 +1,14 @@ /* eslint-disable @typescript-eslint/no-non-null-assertion */ import {selectOrCreateApp} from './dev/select-app.js' -import {fetchOrgFromId, fetchOrganizations, fetchStoreByDomain} from './dev/fetch.js' -import {convertToTransferDisabledStoreIfNeeded, selectStore} from './dev/select-store.js' +import {fetchOrgFromId, fetchOrganizations} from './dev/fetch.js' import {ensureDeploymentIdsPresence} from './context/identifiers.js' import {createExtension} from './dev/create-extension.js' -import {CachedAppInfo, clearCachedAppInfo, getCachedAppInfo, setCachedAppInfo} from './local-storage.js' +import {CachedAppInfo, clearCachedAppInfo, getCachedAppInfo} from './local-storage.js' import link from './app/config/link.js' -import {fetchAppRemoteConfiguration} from './app/select-app.js' -import {fetchSpecifications} from './generate/fetch-extension-specifications.js' import {patchAppConfigurationFile} from './app/patch-app-configuration-file.js' import {DeployOptions} from './deploy.js' import {reuseDevConfigPrompt, selectOrganizationPrompt} from '../prompts/dev.js' import { - AppConfiguration, AppInterface, isCurrentAppSchema, CurrentAppConfiguration, @@ -22,13 +18,7 @@ import { import {Identifiers, UuidOnlyIdentifiers, updateAppIdentifiers, getAppIdentifiers} from '../models/app/identifiers.js' import {Organization, OrganizationApp, OrganizationStore} from '../models/organization.js' import metadata from '../metadata.js' -import { - getAppConfigurationFileName, - getAppConfigurationShorthand, - loadApp, - loadAppConfiguration, - loadAppName, -} from '../models/app/loader.js' +import {getAppConfigurationFileName} from '../models/app/loader.js' import {ExtensionInstance} from '../models/extensions/extension-instance.js' import {ExtensionRegistration} from '../api/graphql/all_app_extension_registrations.js' @@ -39,10 +29,8 @@ import { import {DeveloperPlatformClient, selectDeveloperPlatformClient} from '../utilities/developer-platform-client.js' import {tryParseInt} from '@shopify/cli-kit/common/string' import {Token, TokenItem, renderConfirmationPrompt, renderInfo} from '@shopify/cli-kit/node/ui' -import {partnersFqdn} from '@shopify/cli-kit/node/context/fqdn' import {AbortError} from '@shopify/cli-kit/node/error' import {outputContent} from '@shopify/cli-kit/node/output' -import {getOrganization} from '@shopify/cli-kit/node/environment' import {basename, joinPath, sniffForJson} from '@shopify/cli-kit/node/path' import {glob} from '@shopify/cli-kit/node/fs' @@ -53,149 +41,6 @@ export const InvalidApiKeyErrorMessage = (apiKey: string) => { } } -export interface DevContextOptions { - directory: string - apiKey?: string - storeFqdn?: string - reset: boolean - developerPlatformClient: DeveloperPlatformClient - customInfoBox?: boolean -} - -interface DevContextOutput { - remoteApp: Omit & {apiSecret?: string} - remoteAppUpdated: boolean - storeFqdn: string - storeId: string - updateURLs: boolean | undefined - localApp: AppInterface - organization?: string - configFile?: string -} - -/** - * Make sure there is a valid context to execute `dev` - * That means we have a valid organization, app and dev store selected. - * - * If there are app/store from flags, we check if they are valid. If they are not, throw an error. - * If there is info in the cache or current configuration, check if it is still valid and return it. - * If there is no info (or is invalid): - * - Show prompts to select an org, app and dev store - * - The info will be updated in the cache or current configuration - * - * @param options - Current dev context options - * @returns The selected org, app and dev store - */ -export async function ensureDevContext(options: DevContextOptions): Promise { - let developerPlatformClient = options.developerPlatformClient - const {configuration, cachedInfo, remoteApp} = await getAppContext({ - ...options, - enableLinkingPrompt: !options.apiKey, - }) - developerPlatformClient = remoteApp?.developerPlatformClient ?? developerPlatformClient - - let orgId = getOrganization() || cachedInfo?.orgId - if (!orgId) { - const org = await selectOrg() - developerPlatformClient = selectDeveloperPlatformClient({organization: org}) - orgId = org.id - } - - const organization = await fetchOrgFromId(orgId, developerPlatformClient) - - // we select an app or a dev store from a command flag - let {app: selectedApp, store: selectedStore} = await fetchDevDataFromOptions(options, orgId, developerPlatformClient) - - // if no stores or apps were selected previously from a command, - // we try to load the app or the dev store from the current config or cache - // if that's not available, we prompt the user to choose an existing one or create a new one - if (!selectedApp || !selectedStore) { - const [cachedApp, cachedStore] = await Promise.all([ - selectedApp || - remoteApp || - (cachedInfo?.appId && - appFromId({id: cachedInfo.appGid, apiKey: cachedInfo.appId, organizationId: orgId, developerPlatformClient})), - selectedStore || (cachedInfo?.storeFqdn && storeFromFqdn(cachedInfo.storeFqdn, orgId, developerPlatformClient)), - ]) - - if (cachedApp) { - selectedApp = cachedApp - } else { - const {apps, hasMorePages} = await developerPlatformClient.appsForOrg(orgId) - // get toml names somewhere close to here - const localAppName = await loadAppName(options.directory) - selectedApp = await selectOrCreateApp(localAppName, apps, hasMorePages, organization, developerPlatformClient) - } - - if (cachedStore) { - selectedStore = cachedStore - } else { - const allStores = await developerPlatformClient.devStoresForOrg(orgId) - selectedStore = await selectStore(allStores, organization, developerPlatformClient) - } - } - - const specifications = await fetchSpecifications({developerPlatformClient, app: selectedApp}) - - selectedApp = { - ...selectedApp, - configuration: await fetchAppRemoteConfiguration( - selectedApp, - developerPlatformClient, - specifications, - selectedApp.flags, - ), - } - - const localApp = await loadApp({ - directory: options.directory, - specifications, - userProvidedConfigName: getAppConfigurationShorthand(configuration.path), - remoteFlags: selectedApp.flags, - }) - - // We only update the cache or config if the current app is the right one - const rightApp = selectedApp.apiKey === cachedInfo?.appId - if (isCurrentAppSchema(configuration) && rightApp) { - if (cachedInfo) cachedInfo.storeFqdn = selectedStore?.shopDomain - const newConfiguration = { - ...configuration, - build: { - ...configuration.build, - dev_store_url: selectedStore?.shopDomain, - }, - } - localApp.configuration = newConfiguration - - const patch = {build: {dev_store_url: selectedStore?.shopDomain}} - await patchAppConfigurationFile({path: configuration.path, patch, schema: localApp.configSchema}) - } else if (!cachedInfo || rightApp) { - setCachedAppInfo({ - appId: selectedApp.apiKey, - title: selectedApp.title, - directory: options.directory, - storeFqdn: selectedStore?.shopDomain, - orgId, - }) - } - - // if (!options.customInfoBox) { - // showReusedDevValues({ - // appName: selectedApp.title, - // selectedStore, - // cachedInfo, - // organization, - // }) - // } - - const result = buildOutput(selectedApp, selectedStore, localApp, cachedInfo, organization.businessName) - await logMetadataForLoadedContext({ - organizationId: result.remoteApp.organizationId, - apiKey: result.remoteApp.apiKey, - }) - return result -} - export const resetHelpMessage: Token[] = [ 'You can pass ', {command: '--reset'}, @@ -229,43 +74,6 @@ export const appFromId = async (options: AppFromIdOptions): Promise => { - const result = await fetchStoreByDomain(orgId, storeFqdn, developerPlatformClient) - if (result?.store) { - // never automatically convert a store provided via the cache - await convertToTransferDisabledStoreIfNeeded(result.store, orgId, developerPlatformClient, 'never') - return result.store - } else { - throw new AbortError(`Couldn't find the store with domain "${storeFqdn}".`, resetHelpMessage) - } -} - -function buildOutput( - app: OrganizationApp, - store: OrganizationStore, - localApp: AppInterface, - cachedInfo?: CachedAppInfo, - organization?: string, -): DevContextOutput { - return { - remoteApp: { - ...app, - apiSecret: app.apiSecretKeys.length === 0 ? undefined : app.apiSecretKeys[0]!.secret, - }, - remoteAppUpdated: app.apiKey !== cachedInfo?.previousAppId, - storeFqdn: store.shopDomain, - storeId: store.shopId, - updateURLs: cachedInfo?.updateURLs, - localApp, - organization, - configFile: cachedInfo?.configFile, - } -} - /** * If there is a cached ApiKey used for dev, retrieve that and ask the user if they want to reuse it * @param app - The local app object @@ -484,123 +292,6 @@ export async function fetchAppAndIdentifiers( return [remoteApp, envIdentifiers] } -/** - * Any data sent via input flags takes precedence and needs to be validated. - * If any of the inputs is invalid, we must throw an error and stop the execution. - */ -async function fetchDevDataFromOptions( - options: DevContextOptions, - orgId: string, - developerPlatformClient: DeveloperPlatformClient, -): Promise<{app?: OrganizationApp; store?: OrganizationStore}> { - const [selectedApp, orgWithStore] = await Promise.all([ - (async () => { - let selectedApp: OrganizationApp | undefined - if (options.apiKey) { - selectedApp = await appFromId({apiKey: options.apiKey, developerPlatformClient}) - if (!selectedApp) { - const errorMessage = InvalidApiKeyErrorMessage(options.apiKey) - throw new AbortError(errorMessage.message, errorMessage.tryMessage) - } - return selectedApp - } - })(), - (async () => { - if (options.storeFqdn) { - const orgWithStore = await fetchStoreByDomain(orgId, options.storeFqdn, developerPlatformClient) - if (!orgWithStore) throw new AbortError(`Could not find Organization for id ${orgId}.`) - if (!orgWithStore.store) { - const partners = await partnersFqdn() - const org = orgWithStore.organization - throw new AbortError( - `Could not find ${options.storeFqdn} in the Organization ${org.businessName} as a valid store.`, - `Visit https://${partners}/${org.id}/stores to create a new development or Shopify Plus sandbox store in your organization`, - ) - } - return orgWithStore as {store: OrganizationStore; organization: Organization} - } - })(), - ]) - let selectedStore: OrganizationStore | undefined - - if (options.storeFqdn) { - selectedStore = orgWithStore!.store - // never automatically convert a store provided via the command line - await convertToTransferDisabledStoreIfNeeded( - selectedStore, - orgWithStore!.organization.id, - developerPlatformClient, - 'never', - ) - } - - return {app: selectedApp, store: selectedStore} -} - -interface AppContext { - configuration: AppConfiguration - cachedInfo?: CachedAppInfo - remoteApp?: OrganizationApp -} - -/** - * Retrieve app info from the cache or the current configuration. - * - * @param reset - Whether to reset the cache or not. - * @param directory - The directory containing the app. - * @param developerPlatformClient - The client to access the platform API - */ -async function getAppContext({ - reset, - directory, - configName, - enableLinkingPrompt = true, -}: { - reset: boolean - directory: string - configName?: string - enableLinkingPrompt?: boolean -}): Promise { - await linkIfNecessary(directory, reset, enableLinkingPrompt) - - let cachedInfo = getCachedAppInfo(directory) - - const {configuration} = await loadAppConfiguration({ - directory, - userProvidedConfigName: configName, - }) - - const developerPlatformClient = selectDeveloperPlatformClient({configuration}) - - let remoteApp - if (isCurrentAppSchema(configuration)) { - remoteApp = await appFromId({ - apiKey: configuration.client_id, - id: configuration.app_id, - organizationId: configuration.organization_id, - developerPlatformClient, - }) - cachedInfo = { - ...cachedInfo, - directory, - configFile: basename(configuration.path), - orgId: remoteApp.organizationId, - appId: remoteApp.apiKey, - title: remoteApp.title, - storeFqdn: configuration.build?.dev_store_url, - updateURLs: configuration.build?.automatically_update_urls_on_dev, - } - - await logMetadataForLoadedContext({organizationId: remoteApp.organizationId, apiKey: remoteApp.apiKey}) - } - - return { - configuration, - cachedInfo, - remoteApp, - } -} - async function linkIfNecessary( directory: string, reset: boolean, From 4117229dee6ea17d4122497d3fc080db45074874 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 18 Oct 2024 11:46:10 +0200 Subject: [PATCH 08/15] Update store-context tests --- packages/app/src/cli/services/store-context.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/app/src/cli/services/store-context.test.ts b/packages/app/src/cli/services/store-context.test.ts index 3928049a803..6d916adfe94 100644 --- a/packages/app/src/cli/services/store-context.test.ts +++ b/packages/app/src/cli/services/store-context.test.ts @@ -1,6 +1,6 @@ import {storeContext} from './store-context.js' import {fetchStore} from './dev/fetch.js' -import {selectStore} from './dev/select-store.js' +import {convertToTransferDisabledStoreIfNeeded, selectStore} from './dev/select-store.js' import {LoadedAppContextOutput} from './app-context.js' import { testAppLinked, @@ -49,6 +49,12 @@ describe('storeContext', () => { 'explicit-store.myshopify.com', mockDeveloperPlatformClient, ) + expect(convertToTransferDisabledStoreIfNeeded).toHaveBeenCalledWith( + mockStore, + mockOrganization.id, + mockDeveloperPlatformClient, + 'never', + ) expect(result).toEqual(mockStore) }) From 8ceb4f8abec0cf084ea5a936b5c3fb9d6ab55859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 18 Oct 2024 12:11:58 +0200 Subject: [PATCH 09/15] Update urls tests --- .../app/src/cli/services/dev/urls.test.ts | 69 ++----------------- 1 file changed, 7 insertions(+), 62 deletions(-) diff --git a/packages/app/src/cli/services/dev/urls.test.ts b/packages/app/src/cli/services/dev/urls.test.ts index b0874807f27..07cf37c7da7 100644 --- a/packages/app/src/cli/services/dev/urls.test.ts +++ b/packages/app/src/cli/services/dev/urls.test.ts @@ -10,11 +10,11 @@ import { import { DEFAULT_CONFIG, testApp, + testAppLinked, testAppWithConfig, testDeveloperPlatformClient, } from '../../models/app/app.test-data.js' import {UpdateURLsVariables} from '../../api/graphql/update_urls.js' -import {setCachedAppInfo} from '../local-storage.js' import {patchAppConfigurationFile} from '../app/patch-app-configuration-file.js' import {beforeEach, describe, expect, vi, test} from 'vitest' import {AbortError} from '@shopify/cli-kit/node/error' @@ -216,6 +216,7 @@ describe('shouldOrPromptUpdateURLs', () => { appDirectory: '/path', newApp: true, apiKey: 'api-key', + localApp: testAppLinked({configuration: {...DEFAULT_CONFIG, client_id: 'api-key'}}), } // When @@ -225,44 +226,13 @@ describe('shouldOrPromptUpdateURLs', () => { expect(got).toEqual(true) }) - test('returns true if the cached value is true (always)', async () => { - // Given - const options = { - currentURLs, - appDirectory: '/path', - cachedUpdateURLs: true, - apiKey: 'api-key', - } - - // When - const got = await shouldOrPromptUpdateURLs(options) - - // Then - expect(got).toEqual(true) - }) - - test('returns false if the cached value is false (never)', async () => { - // Given - const options = { - currentURLs, - appDirectory: '/path', - cachedUpdateURLs: false, - apiKey: 'api-key', - } - - // When - const got = await shouldOrPromptUpdateURLs(options) - - // Then - expect(got).toEqual(false) - }) - test('returns true when the user selects yes', async () => { // Given const options = { currentURLs, appDirectory: '/path', apiKey: 'api-key', + localApp: testAppLinked({configuration: {...DEFAULT_CONFIG, client_id: 'api-key'}}), } vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) @@ -279,6 +249,7 @@ describe('shouldOrPromptUpdateURLs', () => { currentURLs, appDirectory: '/path', apiKey: 'api-key', + localApp: testAppLinked({configuration: {...DEFAULT_CONFIG, client_id: 'api-key'}}), } vi.mocked(renderConfirmationPrompt).mockResolvedValue(false) @@ -289,32 +260,13 @@ describe('shouldOrPromptUpdateURLs', () => { expect(got).toEqual(false) }) - test('saves the response for the next time', async () => { - // Given - const options = { - currentURLs, - appDirectory: '/path', - apiKey: 'api-key', - } - vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) - - // When - await shouldOrPromptUpdateURLs(options) - - // Then - expect(setCachedAppInfo).toHaveBeenNthCalledWith(1, { - directory: '/path', - updateURLs: true, - }) - }) - - test('does not update config file or cache if current config client does not match remote', async () => { + test('does not update config file if current config client does not match remote', async () => { // Given const options = { currentURLs, appDirectory: '/path', apiKey: 'api-key', - localApp: testApp({configuration: {...DEFAULT_CONFIG, client_id: 'different'}}, 'current'), + localApp: testAppLinked({configuration: {...DEFAULT_CONFIG, client_id: 'different'}}), } vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) @@ -323,13 +275,12 @@ describe('shouldOrPromptUpdateURLs', () => { // Then expect(result).toBe(true) - expect(setCachedAppInfo).not.toHaveBeenCalled() expect(patchAppConfigurationFile).not.toHaveBeenCalled() }) test('updates the config file if current config client matches remote', async () => { // Given - const localApp = testApp({configuration: {...DEFAULT_CONFIG, client_id: 'api-key'}}, 'current') + const localApp = testAppLinked({configuration: {...DEFAULT_CONFIG, client_id: 'api-key'}}) const options = { currentURLs, appDirectory: '/path', @@ -343,7 +294,6 @@ describe('shouldOrPromptUpdateURLs', () => { // Then expect(result).toBe(true) - expect(setCachedAppInfo).not.toHaveBeenCalled() expect(patchAppConfigurationFile).toHaveBeenCalledWith( expect.objectContaining({ path: localApp.configuration.path, @@ -439,7 +389,6 @@ describe('generateFrontendURL', () => { // Then expect(got).toEqual({frontendUrl: 'https://4040-gitpod.url.fqdn.com', frontendPort: 4040, usingLocalhost: false}) - expect(setCachedAppInfo).not.toBeCalled() expect(renderSelectPrompt).not.toBeCalled() }) @@ -457,7 +406,6 @@ describe('generateFrontendURL', () => { frontendPort: 4040, usingLocalhost: false, }) - expect(setCachedAppInfo).not.toBeCalled() expect(renderSelectPrompt).not.toBeCalled() }) @@ -477,7 +425,6 @@ describe('generateFrontendURL', () => { frontendPort: 4040, usingLocalhost: false, }) - expect(setCachedAppInfo).not.toBeCalled() expect(renderSelectPrompt).not.toBeCalled() }) @@ -497,7 +444,6 @@ describe('generateFrontendURL', () => { frontendPort: 1234, usingLocalhost: false, }) - expect(setCachedAppInfo).not.toBeCalled() expect(renderSelectPrompt).not.toBeCalled() }) @@ -518,7 +464,6 @@ describe('generateFrontendURL', () => { frontendPort: 4040, usingLocalhost: false, }) - expect(setCachedAppInfo).not.toBeCalled() expect(renderSelectPrompt).not.toBeCalled() }) From 3e980d95aa95678e7d6619de06eb7740ffd5b119 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 18 Oct 2024 12:12:44 +0200 Subject: [PATCH 10/15] revert changes to urls.ts --- .../app/src/cli/services/dev/urls.test.ts | 69 +++++++++++++++++-- packages/app/src/cli/services/dev/urls.ts | 23 ++++--- 2 files changed, 76 insertions(+), 16 deletions(-) diff --git a/packages/app/src/cli/services/dev/urls.test.ts b/packages/app/src/cli/services/dev/urls.test.ts index 07cf37c7da7..b0874807f27 100644 --- a/packages/app/src/cli/services/dev/urls.test.ts +++ b/packages/app/src/cli/services/dev/urls.test.ts @@ -10,11 +10,11 @@ import { import { DEFAULT_CONFIG, testApp, - testAppLinked, testAppWithConfig, testDeveloperPlatformClient, } from '../../models/app/app.test-data.js' import {UpdateURLsVariables} from '../../api/graphql/update_urls.js' +import {setCachedAppInfo} from '../local-storage.js' import {patchAppConfigurationFile} from '../app/patch-app-configuration-file.js' import {beforeEach, describe, expect, vi, test} from 'vitest' import {AbortError} from '@shopify/cli-kit/node/error' @@ -216,7 +216,6 @@ describe('shouldOrPromptUpdateURLs', () => { appDirectory: '/path', newApp: true, apiKey: 'api-key', - localApp: testAppLinked({configuration: {...DEFAULT_CONFIG, client_id: 'api-key'}}), } // When @@ -226,13 +225,44 @@ describe('shouldOrPromptUpdateURLs', () => { expect(got).toEqual(true) }) + test('returns true if the cached value is true (always)', async () => { + // Given + const options = { + currentURLs, + appDirectory: '/path', + cachedUpdateURLs: true, + apiKey: 'api-key', + } + + // When + const got = await shouldOrPromptUpdateURLs(options) + + // Then + expect(got).toEqual(true) + }) + + test('returns false if the cached value is false (never)', async () => { + // Given + const options = { + currentURLs, + appDirectory: '/path', + cachedUpdateURLs: false, + apiKey: 'api-key', + } + + // When + const got = await shouldOrPromptUpdateURLs(options) + + // Then + expect(got).toEqual(false) + }) + test('returns true when the user selects yes', async () => { // Given const options = { currentURLs, appDirectory: '/path', apiKey: 'api-key', - localApp: testAppLinked({configuration: {...DEFAULT_CONFIG, client_id: 'api-key'}}), } vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) @@ -249,7 +279,6 @@ describe('shouldOrPromptUpdateURLs', () => { currentURLs, appDirectory: '/path', apiKey: 'api-key', - localApp: testAppLinked({configuration: {...DEFAULT_CONFIG, client_id: 'api-key'}}), } vi.mocked(renderConfirmationPrompt).mockResolvedValue(false) @@ -260,13 +289,32 @@ describe('shouldOrPromptUpdateURLs', () => { expect(got).toEqual(false) }) - test('does not update config file if current config client does not match remote', async () => { + test('saves the response for the next time', async () => { + // Given + const options = { + currentURLs, + appDirectory: '/path', + apiKey: 'api-key', + } + vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) + + // When + await shouldOrPromptUpdateURLs(options) + + // Then + expect(setCachedAppInfo).toHaveBeenNthCalledWith(1, { + directory: '/path', + updateURLs: true, + }) + }) + + test('does not update config file or cache if current config client does not match remote', async () => { // Given const options = { currentURLs, appDirectory: '/path', apiKey: 'api-key', - localApp: testAppLinked({configuration: {...DEFAULT_CONFIG, client_id: 'different'}}), + localApp: testApp({configuration: {...DEFAULT_CONFIG, client_id: 'different'}}, 'current'), } vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) @@ -275,12 +323,13 @@ describe('shouldOrPromptUpdateURLs', () => { // Then expect(result).toBe(true) + expect(setCachedAppInfo).not.toHaveBeenCalled() expect(patchAppConfigurationFile).not.toHaveBeenCalled() }) test('updates the config file if current config client matches remote', async () => { // Given - const localApp = testAppLinked({configuration: {...DEFAULT_CONFIG, client_id: 'api-key'}}) + const localApp = testApp({configuration: {...DEFAULT_CONFIG, client_id: 'api-key'}}, 'current') const options = { currentURLs, appDirectory: '/path', @@ -294,6 +343,7 @@ describe('shouldOrPromptUpdateURLs', () => { // Then expect(result).toBe(true) + expect(setCachedAppInfo).not.toHaveBeenCalled() expect(patchAppConfigurationFile).toHaveBeenCalledWith( expect.objectContaining({ path: localApp.configuration.path, @@ -389,6 +439,7 @@ describe('generateFrontendURL', () => { // Then expect(got).toEqual({frontendUrl: 'https://4040-gitpod.url.fqdn.com', frontendPort: 4040, usingLocalhost: false}) + expect(setCachedAppInfo).not.toBeCalled() expect(renderSelectPrompt).not.toBeCalled() }) @@ -406,6 +457,7 @@ describe('generateFrontendURL', () => { frontendPort: 4040, usingLocalhost: false, }) + expect(setCachedAppInfo).not.toBeCalled() expect(renderSelectPrompt).not.toBeCalled() }) @@ -425,6 +477,7 @@ describe('generateFrontendURL', () => { frontendPort: 4040, usingLocalhost: false, }) + expect(setCachedAppInfo).not.toBeCalled() expect(renderSelectPrompt).not.toBeCalled() }) @@ -444,6 +497,7 @@ describe('generateFrontendURL', () => { frontendPort: 1234, usingLocalhost: false, }) + expect(setCachedAppInfo).not.toBeCalled() expect(renderSelectPrompt).not.toBeCalled() }) @@ -464,6 +518,7 @@ describe('generateFrontendURL', () => { frontendPort: 4040, usingLocalhost: false, }) + expect(setCachedAppInfo).not.toBeCalled() expect(renderSelectPrompt).not.toBeCalled() }) diff --git a/packages/app/src/cli/services/dev/urls.ts b/packages/app/src/cli/services/dev/urls.ts index bdd571af2c0..42d8564b776 100644 --- a/packages/app/src/cli/services/dev/urls.ts +++ b/packages/app/src/cli/services/dev/urls.ts @@ -1,11 +1,12 @@ import {updateURLsPrompt} from '../../prompts/dev.js' import { AppConfigurationInterface, - AppLinkedInterface, + AppInterface, CurrentAppConfiguration, isCurrentAppSchema, } from '../../models/app/app.js' import {UpdateURLsSchema, UpdateURLsVariables} from '../../api/graphql/update_urls.js' +import {setCachedAppInfo} from '../local-storage.js' import {AppConfigurationUsedByCli} from '../../models/extensions/specifications/types/app_config.js' import {DeveloperPlatformClient} from '../../utilities/developer-platform-client.js' import {patchAppConfigurationFile} from '../app/patch-app-configuration-file.js' @@ -242,7 +243,7 @@ interface ShouldOrPromptUpdateURLsOptions { appDirectory: string cachedUpdateURLs?: boolean newApp?: boolean - localApp: AppLinkedInterface + localApp?: AppInterface apiKey: string } @@ -257,14 +258,18 @@ export async function shouldOrPromptUpdateURLs(options: ShouldOrPromptUpdateURLs options.currentURLs.redirectUrlWhitelist, ) - const localConfiguration = options.localApp.configuration - localConfiguration.build = { - ...localConfiguration.build, - automatically_update_urls_on_dev: shouldUpdateURLs, + if (options.localApp && isCurrentAppSchema(options.localApp.configuration)) { + const localConfiguration = options.localApp.configuration + localConfiguration.build = { + ...localConfiguration.build, + automatically_update_urls_on_dev: shouldUpdateURLs, + } + const patch = {build: {automatically_update_urls_on_dev: shouldUpdateURLs}} + const path = options.localApp.configuration.path + await patchAppConfigurationFile({path, patch, schema: options.localApp.configSchema}) + } else { + setCachedAppInfo({directory: options.appDirectory, updateURLs: shouldUpdateURLs}) } - const patch = {build: {automatically_update_urls_on_dev: shouldUpdateURLs}} - const path = options.localApp.configuration.path - await patchAppConfigurationFile({path, patch, schema: options.localApp.configSchema}) } return shouldUpdateURLs } From 9d70ed88b205b62cfc2eff58832555139306f0c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 18 Oct 2024 12:14:38 +0200 Subject: [PATCH 11/15] Update app-context test --- packages/app/src/cli/services/app-context.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/app/src/cli/services/app-context.test.ts b/packages/app/src/cli/services/app-context.test.ts index 3837a0ee2f0..91ef8dfae20 100644 --- a/packages/app/src/cli/services/app-context.test.ts +++ b/packages/app/src/cli/services/app-context.test.ts @@ -230,6 +230,7 @@ describe('linkedAppContext', () => { expect.objectContaining({ partner_id: tryParseInt(mockRemoteApp.organizationId), api_key: mockRemoteApp.apiKey, + cmd_app_reset_used: false, }), ) }) From 6e797a3d2783f6bf719c62a33c580eff234a2de1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 18 Oct 2024 12:21:57 +0200 Subject: [PATCH 12/15] Return the linked app in dev command --- packages/app/src/cli/commands/app/dev.ts | 4 ++-- packages/app/src/cli/services/dev.ts | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index 39a37d5061b..3b8014984d4 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -194,7 +194,7 @@ If you're using the PHP or Ruby app template, then you need to complete the foll graphiqlKey: flags['graphiql-key'], } - const result = await dev(devOptions) - return {app: result.app} + await dev(devOptions) + return {app: appContextResult.app} } } diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index 5db2ba095c6..eeffc7a30b1 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -74,7 +74,6 @@ export async function dev(commandOptions: DevOptions) { const {processes, graphiqlUrl, previewUrl} = await setupDevProcesses(config) await actionsBeforeLaunchingDevProcesses(config) await launchDevProcesses({processes, previewUrl, graphiqlUrl, config}) - return {app: config.localApp} } async function prepareForDev(commandOptions: DevOptions): Promise { From b7e8be266ee296bd55e922ae705dd11f0f973b5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 18 Oct 2024 12:45:36 +0200 Subject: [PATCH 13/15] Update setup-dev-processes test --- .../dev/processes/setup-dev-processes.test.ts | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts index 63cfdb8bcb8..21ebb294886 100644 --- a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts +++ b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts @@ -18,6 +18,10 @@ import { testUIExtension, testFunctionExtension, testWebhookExtensions, + testOrganizationApp, + testAppLinked, + testOrganization, + testOrganizationStore, } from '../../../models/app/app.test-data.js' import {WebType} from '../../../models/app/app.js' import {ensureDeploymentIdsPresence} from '../../context/identifiers.js' @@ -61,6 +65,15 @@ beforeEach(() => { }) }) +const appContextResult = { + app: testAppLinked(), + remoteApp: testOrganizationApp(), + developerPlatformClient: testDeveloperPlatformClient(), + organization: testOrganization(), + store: testOrganizationStore({}), + specifications: [], +} + describe('setup-dev-processes', () => { test('can create a process list', async () => { const developerPlatformClient: DeveloperPlatformClient = testDeveloperPlatformClient() @@ -69,11 +82,11 @@ describe('setup-dev-processes', () => { const remoteAppUpdated = true const graphiqlPort = 1234 const commandOptions: DevConfig['commandOptions'] = { + ...appContextResult, subscriptionProductUrl: '/products/999999', checkoutCartUrl: '/cart/999999:1', theme: '1', directory: '', - reset: false, update: false, commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, @@ -266,8 +279,8 @@ describe('setup-dev-processes', () => { const remoteAppUpdated = true const graphiqlPort = 1234 const commandOptions: DevConfig['commandOptions'] = { + ...appContextResult, directory: '', - reset: false, update: false, commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, @@ -334,11 +347,11 @@ describe('setup-dev-processes', () => { const remoteAppUpdated = true const graphiqlPort = 1234 const commandOptions: DevConfig['commandOptions'] = { + ...appContextResult, subscriptionProductUrl: '/products/999999', checkoutCartUrl: '/cart/999999:1', theme: '1', directory: '', - reset: false, update: false, commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, @@ -428,11 +441,11 @@ describe('setup-dev-processes', () => { const remoteAppUpdated = true const graphiqlPort = 1234 const commandOptions: DevConfig['commandOptions'] = { + ...appContextResult, subscriptionProductUrl: '/products/999999', checkoutCartUrl: '/cart/999999:1', theme: '1', directory: '', - reset: false, update: false, commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, @@ -510,11 +523,11 @@ describe('setup-dev-processes', () => { const remoteAppUpdated = true const graphiqlPort = 1234 const commandOptions: DevConfig['commandOptions'] = { + ...appContextResult, subscriptionProductUrl: '/products/999999', checkoutCartUrl: '/cart/999999:1', theme: '1', directory: '', - reset: false, update: false, commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, From 73c0000f34b168ffc2d0325b76872f188f96b11f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Thu, 24 Oct 2024 10:03:07 +0200 Subject: [PATCH 14/15] Fix metadata and store fetching --- packages/app/src/cli/commands/app/dev.ts | 1 + packages/app/src/cli/metadata.ts | 6 ++++-- .../app/src/cli/services/store-context.ts | 20 ++++++++++++++++++- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index 3b8014984d4..a38a6271b40 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -173,6 +173,7 @@ If you're using the PHP or Ruby app template, then you need to complete the foll const store = await storeContext({ appContextResult, + storeFqdn: flags.store, forceReselectStore: flags.reset, }) diff --git a/packages/app/src/cli/metadata.ts b/packages/app/src/cli/metadata.ts index 0dc180a4024..79f11dbde89 100644 --- a/packages/app/src/cli/metadata.ts +++ b/packages/app/src/cli/metadata.ts @@ -9,10 +9,12 @@ type CmdFieldsFromMonorail = PickByPrefix & PickByPrefix & PickByPrefix & - PickByPrefix + PickByPrefix & + PickByPrefix type CmdSensitiveFieldsFromMonorail = PickByPrefix & - PickByPrefix + PickByPrefix & + PickByPrefix const metadata = createRuntimeMetadataContainer< { diff --git a/packages/app/src/cli/services/store-context.ts b/packages/app/src/cli/services/store-context.ts index ce33202dcf5..c9d2ea9df88 100644 --- a/packages/app/src/cli/services/store-context.ts +++ b/packages/app/src/cli/services/store-context.ts @@ -2,6 +2,9 @@ import {fetchStore} from './dev/fetch.js' import {convertToTransferDisabledStoreIfNeeded, selectStore} from './dev/select-store.js' import {LoadedAppContextOutput} from './app-context.js' import {OrganizationStore} from '../models/organization.js' +import metadata from '../metadata.js' +import {hashString} from '@shopify/cli-kit/node/crypto' +import {normalizeStoreFqdn} from '@shopify/cli-kit/node/context/fqdn' /** * Input options for the `storeContext` function. @@ -35,7 +38,8 @@ export async function storeContext({ const cachedStoreInToml = forceReselectStore ? undefined : app.configuration.build?.dev_store_url // An explicit storeFqdn has preference over anything else. - const storeFqdnToUse = storeFqdn || cachedStoreInToml + const storeFqdnToUse = storeFqdn ?? cachedStoreInToml + if (storeFqdnToUse) { selectedStore = await fetchStore(organization, storeFqdnToUse, developerPlatformClient) // never automatically convert a store provided via the command line @@ -46,5 +50,19 @@ export async function storeContext({ selectedStore = await selectStore(allStores, organization, developerPlatformClient) } + await logMetadata(selectedStore, forceReselectStore) + selectedStore.shopDomain = await normalizeStoreFqdn(selectedStore.shopDomain) + return selectedStore } + +async function logMetadata(selectedStore: OrganizationStore, resetUsed: boolean) { + await metadata.addPublicMetadata(() => ({ + cmd_app_reset_used: resetUsed, + store_fqdn_hash: hashString(selectedStore.shopDomain), + })) + + await metadata.addSensitiveMetadata(() => ({ + store_fqdn: selectedStore.shopDomain, + })) +} From 090246c4e669f0b3e9c7cd79a29d891a52dffd92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Thu, 24 Oct 2024 10:05:19 +0200 Subject: [PATCH 15/15] Add tests for logMetadata in store-context --- .../src/cli/services/store-context.test.ts | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/packages/app/src/cli/services/store-context.test.ts b/packages/app/src/cli/services/store-context.test.ts index 6d916adfe94..85cce5c96ce 100644 --- a/packages/app/src/cli/services/store-context.test.ts +++ b/packages/app/src/cli/services/store-context.test.ts @@ -9,7 +9,9 @@ import { testOrganizationApp, testOrganizationStore, } from '../models/app/app.test-data.js' +import metadata from '../metadata.js' import {vi, describe, test, expect} from 'vitest' +import {hashString} from '@shopify/cli-kit/node/crypto' vi.mock('./dev/fetch') vi.mock('./dev/select-store') @@ -121,4 +123,27 @@ describe('storeContext', () => { 'No stores available', ) }) + + test('calls logMetadata', async () => { + // Given + vi.mocked(fetchStore).mockResolvedValue(mockStore) + + // When + await storeContext({appContextResult, forceReselectStore: false}) + + // Then + const meta = metadata.getAllPublicMetadata() + expect(meta).toEqual( + expect.objectContaining({ + store_fqdn_hash: hashString(mockStore.shopDomain), + }), + ) + + const sensitiveMeta = metadata.getAllSensitiveMetadata() + expect(sensitiveMeta).toEqual( + expect.objectContaining({ + store_fqdn: mockStore.shopDomain, + }), + ) + }) })