From 2e024d579789278011ccaecbc0cdab7b5645c221 Mon Sep 17 00:00:00 2001 From: Mitch Dickinson Date: Tue, 23 Jul 2024 13:59:54 -0400 Subject: [PATCH 1/7] Run link during a fresh deploy if necessary --- packages/app/src/cli/services/context.test.ts | 20 ++++++- packages/app/src/cli/services/context.ts | 55 ++++++++++++++----- packages/app/src/cli/services/info.ts | 2 +- 3 files changed, 58 insertions(+), 19 deletions(-) diff --git a/packages/app/src/cli/services/context.test.ts b/packages/app/src/cli/services/context.test.ts index 3b2fee89a8..cc6471bad0 100644 --- a/packages/app/src/cli/services/context.test.ts +++ b/packages/app/src/cli/services/context.test.ts @@ -929,7 +929,7 @@ api_version = "2023-04" }) }) -describe('ensureDeployContext', () => { +describe('ensureDeploy Context', () => { test("fetches the app from the partners' API and returns it alongside the id when identifiers are available locally and the app has no extensions", async () => { // Given const app = testApp() @@ -1194,6 +1194,8 @@ describe('ensureDeployContext', () => { vi.mocked(loadApp).mockResolvedValue(app) vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) vi.mocked(getAppConfigurationFileName).mockReturnValue('shopify.app.toml') + vi.mocked(link).mockResolvedValue(app.configuration) + const writeAppConfigurationFileSpy = vi .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') .mockResolvedValue() @@ -1244,6 +1246,7 @@ describe('ensureDeployContext', () => { vi.mocked(loadApp).mockResolvedValue(app) vi.mocked(renderConfirmationPrompt).mockResolvedValue(false) vi.mocked(getAppConfigurationFileName).mockReturnValue('shopify.app.toml') + vi.mocked(link).mockResolvedValue(app.configuration) const writeAppConfigurationFileSpy = vi .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') .mockResolvedValue() @@ -1288,14 +1291,19 @@ describe('ensureDeployContext', () => { vi.mocked(fetchAppDetailsFromApiKey).mockResolvedValueOnce(APP2) vi.mocked(ensureDeploymentIdsPresence).mockResolvedValue(identifiers) vi.mocked(loadApp).mockResolvedValue(app) + vi.mocked(link).mockResolvedValue((app as any).configuration) + // vi.mocked(selectDeveloperPlatformClient).mockReturnValue(testDeveloperPlatformClient) vi.mocked(getAppConfigurationFileName).mockReturnValue('shopify.app.toml') const writeAppConfigurationFileSpy = vi .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') .mockResolvedValue() const metadataSpyOn = vi.spyOn(metadata, 'addPublicMetadata').mockImplementation(async () => {}) + const options = deployOptions(app) + vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) + // When - await ensureDeployContext(deployOptions(app)) + await ensureDeployContext(options) // Then expect(metadataSpyOn).toHaveBeenNthCalledWith(2, expect.any(Function)) @@ -1344,6 +1352,8 @@ describe('ensureDeployContext', () => { .mockResolvedValue() const metadataSpyOn = vi.spyOn(metadata, 'addPublicMetadata').mockImplementation(async () => {}) + const options = deployOptions(app, true) + vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) // When await ensureDeployContext(deployOptions(app, true)) @@ -1387,14 +1397,18 @@ describe('ensureDeployContext', () => { vi.mocked(fetchAppDetailsFromApiKey).mockResolvedValueOnce(APP2) vi.mocked(ensureDeploymentIdsPresence).mockResolvedValue(identifiers) vi.mocked(loadApp).mockResolvedValue(app) + vi.mocked(link).mockResolvedValue(app.configuration) vi.mocked(renderConfirmationPrompt).mockResolvedValue(false) vi.mocked(getAppConfigurationFileName).mockReturnValue('shopify.app.toml') const writeAppConfigurationFileSpy = vi .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') .mockResolvedValue() + const options = deployOptions(app, false, true) + vi.mocked(selectDeveloperPlatformClient).mockReturnValue(options.developerPlatformClient) + // When - await ensureDeployContext(deployOptions(app, false, true)) + await ensureDeployContext(options) // Then expect(renderConfirmationPrompt).not.toHaveBeenCalled() diff --git a/packages/app/src/cli/services/context.ts b/packages/app/src/cli/services/context.ts index 1c4b6a2380..ec17859410 100644 --- a/packages/app/src/cli/services/context.ts +++ b/packages/app/src/cli/services/context.ts @@ -164,7 +164,7 @@ export async function ensureDevContext(options: DevContextOptions): Promise]> { + let developerPlatformClient = initialDeveloperPlatformClient const app = options.app let reuseDevCache = reuseFromDev let envIdentifiers = getAppIdentifiers({app}, developerPlatformClient) let remoteApp: OrganizationApp | undefined - if (options.reset) { + const {performAppLink, previousCachedInfo} = await decideOnLinkStrategy(app.directory, options.reset, true) + + /** + * TODO: + * - Clean up this refactor to the extent possible (and file an issue for further improvements) + * - Fixing existing tests + * - Add test that link is called on a new deploy + * - More tophatting. + */ + if (performAppLink) { envIdentifiers = {app: undefined, extensions: {}} reuseDevCache = false - const configuration = await link({directory: app.directory, developerPlatformClient}) + const configuration = await link( + { + directory: app.directory, + baseConfigName: previousCachedInfo?.configFile, + developerPlatformClient, + }, + false, + ) app.configuration = configuration + developerPlatformClient = selectDeveloperPlatformClient({configuration}) } if (isCurrentAppSchema(app.configuration)) { @@ -748,24 +766,17 @@ export async function getAppContext({ directory, developerPlatformClient, configName, - promptLinkingApp = true, + enableLinkingPrompt = true, }: { reset: boolean directory: string developerPlatformClient: DeveloperPlatformClient configName?: string - promptLinkingApp?: boolean + enableLinkingPrompt?: boolean }): Promise { - const previousCachedInfo = getCachedAppInfo(directory) - - if (reset) clearCachedAppInfo(directory) - - const firstTimeSetup = previousCachedInfo === undefined - const usingConfigAndResetting = previousCachedInfo?.configFile && reset - const usingConfigWithNoTomls = - previousCachedInfo?.configFile && (await glob(joinPath(directory, 'shopify.app*.toml'))).length === 0 + const {performAppLink, previousCachedInfo} = await decideOnLinkStrategy(directory, reset, enableLinkingPrompt) - if (promptLinkingApp && (firstTimeSetup || usingConfigAndResetting || usingConfigWithNoTomls)) { + if (performAppLink) { await link({directory, baseConfigName: previousCachedInfo?.configFile}, false) } @@ -805,6 +816,20 @@ export async function getAppContext({ } } +async function decideOnLinkStrategy(directory: string, reset: boolean, enableLinkingPrompt: boolean) { + const previousCachedInfo = getCachedAppInfo(directory) + + if (reset) clearCachedAppInfo(directory) + + const firstTimeSetup = previousCachedInfo === undefined + const usingConfigAndResetting = previousCachedInfo?.configFile && reset + const usingConfigWithNoTomls = + previousCachedInfo?.configFile && (await glob(joinPath(directory, 'shopify.app*.toml'))).length === 0 + + const performAppLink = enableLinkingPrompt && (firstTimeSetup || usingConfigAndResetting || usingConfigWithNoTomls) + return {performAppLink, previousCachedInfo} +} + /** * Fetch all orgs the user belongs to and show a prompt to select one of them * @param developerPlatformClient - The client to access the platform API diff --git a/packages/app/src/cli/services/info.ts b/packages/app/src/cli/services/info.ts index 7cd29b42d1..d46eef29c3 100644 --- a/packages/app/src/cli/services/info.ts +++ b/packages/app/src/cli/services/info.ts @@ -131,7 +131,7 @@ class AppInfo { directory: this.app.directory, reset: false, configName: this.options.configName, - promptLinkingApp: false, + enableLinkingPrompt: false, }) developerPlatformClient = remoteApp?.developerPlatformClient ?? developerPlatformClient From 58e3814574346af6a2192c305d2908579c3b7f63 Mon Sep 17 00:00:00 2001 From: Mitch Dickinson Date: Wed, 24 Jul 2024 09:01:38 -0400 Subject: [PATCH 2/7] only call link if were in a deploy context --- packages/app/src/cli/services/context.test.ts | 1 + packages/app/src/cli/services/context.ts | 27 ++++++++++++------- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/packages/app/src/cli/services/context.test.ts b/packages/app/src/cli/services/context.test.ts index cc6471bad0..0e0e49abd9 100644 --- a/packages/app/src/cli/services/context.test.ts +++ b/packages/app/src/cli/services/context.test.ts @@ -1247,6 +1247,7 @@ describe('ensureDeploy Context', () => { vi.mocked(renderConfirmationPrompt).mockResolvedValue(false) vi.mocked(getAppConfigurationFileName).mockReturnValue('shopify.app.toml') vi.mocked(link).mockResolvedValue(app.configuration) + // vi.mocked(selectDeveloperPlatformClient).mockReturnValue(testDeveloperPlatformClient) const writeAppConfigurationFileSpy = vi .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') .mockResolvedValue() diff --git a/packages/app/src/cli/services/context.ts b/packages/app/src/cli/services/context.ts index ec17859410..e53c2a2742 100644 --- a/packages/app/src/cli/services/context.ts +++ b/packages/app/src/cli/services/context.ts @@ -427,7 +427,9 @@ export interface DeployContextOptions { export async function ensureDeployContext(options: DeployContextOptions): Promise { const {reset, force, noRelease} = options let developerPlatformClient = options.developerPlatformClient - const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient) + // do the link here + const performAppLink = await decideOnLinkStrategy(options.app.directory, options.reset, true) + const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient, performAppLink) developerPlatformClient = remoteApp.developerPlatformClient ?? developerPlatformClient const specifications = await fetchSpecifications({developerPlatformClient, app: remoteApp}) @@ -638,6 +640,7 @@ export async function fetchAppAndIdentifiers( }, initialDeveloperPlatformClient: DeveloperPlatformClient, reuseFromDev = true, + performAppLink = false, ): Promise<[OrganizationApp, Partial]> { let developerPlatformClient = initialDeveloperPlatformClient const app = options.app @@ -645,7 +648,7 @@ export async function fetchAppAndIdentifiers( let envIdentifiers = getAppIdentifiers({app}, developerPlatformClient) let remoteApp: OrganizationApp | undefined - const {performAppLink, previousCachedInfo} = await decideOnLinkStrategy(app.directory, options.reset, true) + const previousCachedInfo = getCachedAppInfo(app.directory) /** * TODO: @@ -653,6 +656,7 @@ export async function fetchAppAndIdentifiers( * - Fixing existing tests * - Add test that link is called on a new deploy * - More tophatting. + * - Test dev flow with partners */ if (performAppLink) { envIdentifiers = {app: undefined, extensions: {}} @@ -774,14 +778,13 @@ export async function getAppContext({ configName?: string enableLinkingPrompt?: boolean }): Promise { - const {performAppLink, previousCachedInfo} = await decideOnLinkStrategy(directory, reset, enableLinkingPrompt) + const performAppLink = await decideOnLinkStrategy(directory, reset, enableLinkingPrompt) + let cachedInfo = getCachedAppInfo(directory) if (performAppLink) { - await link({directory, baseConfigName: previousCachedInfo?.configFile}, false) + await link({directory, baseConfigName: cachedInfo?.configFile}, false) } - let cachedInfo = getCachedAppInfo(directory) - const {configuration} = await loadAppConfiguration({ directory, userProvidedConfigName: configName, @@ -816,18 +819,22 @@ export async function getAppContext({ } } -async function decideOnLinkStrategy(directory: string, reset: boolean, enableLinkingPrompt: boolean) { +async function decideOnLinkStrategy( + directory: string, + reset: boolean, + enableLinkingPrompt: boolean, +): Promise { const previousCachedInfo = getCachedAppInfo(directory) if (reset) clearCachedAppInfo(directory) const firstTimeSetup = previousCachedInfo === undefined const usingConfigAndResetting = previousCachedInfo?.configFile && reset - const usingConfigWithNoTomls = - previousCachedInfo?.configFile && (await glob(joinPath(directory, 'shopify.app*.toml'))).length === 0 + const usingConfigWithNoTomls: boolean = + previousCachedInfo?.configFile !== undefined && (await glob(joinPath(directory, 'shopify.app*.toml'))).length === 0 const performAppLink = enableLinkingPrompt && (firstTimeSetup || usingConfigAndResetting || usingConfigWithNoTomls) - return {performAppLink, previousCachedInfo} + return performAppLink } /** From 757fac5f6afff5e4619c91c72e953f6fef000d76 Mon Sep 17 00:00:00 2001 From: Mitch Dickinson Date: Mon, 29 Jul 2024 07:17:38 -0400 Subject: [PATCH 3/7] fixing tests --- packages/app/src/cli/services/context.test.ts | 31 ++++++++++++++----- packages/app/src/cli/services/context.ts | 2 +- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/packages/app/src/cli/services/context.test.ts b/packages/app/src/cli/services/context.test.ts index 0e0e49abd9..96dec7d8d2 100644 --- a/packages/app/src/cli/services/context.test.ts +++ b/packages/app/src/cli/services/context.test.ts @@ -932,7 +932,7 @@ api_version = "2023-04" describe('ensureDeploy Context', () => { test("fetches the app from the partners' API and returns it alongside the id when identifiers are available locally and the app has no extensions", async () => { // Given - const app = testApp() + const app = testAppWithConfig({config: {client_id: APP2.apiKey}}) const identifiers = { app: APP2.apiKey, extensions: {}, @@ -943,6 +943,8 @@ describe('ensureDeploy Context', () => { vi.mocked(fetchAppDetailsFromApiKey).mockResolvedValueOnce(APP2) vi.mocked(ensureDeploymentIdsPresence).mockResolvedValue(identifiers) vi.mocked(loadApp).mockResolvedValue(app) + vi.mocked(link).mockResolvedValue(app.configuration) + vi.mocked(selectDeveloperPlatformClient).mockReturnValue(buildDeveloperPlatformClient()) // When const got = await ensureDeployContext(deployOptions(app)) @@ -998,6 +1000,8 @@ describe('ensureDeploy Context', () => { vi.mocked(getAppIdentifiers).mockReturnValue({app: undefined}) vi.mocked(ensureDeploymentIdsPresence).mockResolvedValue(identifiers) vi.mocked(loadApp).mockResolvedValue(app) + vi.mocked(link).mockResolvedValue(app.configuration) + vi.mocked(selectDeveloperPlatformClient).mockReturnValue(buildDeveloperPlatformClient()) const writeAppConfigurationFileSpy = vi .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') .mockResolvedValue() @@ -1024,7 +1028,7 @@ describe('ensureDeploy Context', () => { test('prompts the user to create or select an app and returns it with its id when the app has no extensions', async () => { // Given - const app = testApp() + const app = testAppWithConfig({config: {client_id: APP2.apiKey}}) const identifiers = { app: APP1.apiKey, extensions: {}, @@ -1035,6 +1039,7 @@ describe('ensureDeploy Context', () => { vi.mocked(fetchAppDetailsFromApiKey).mockResolvedValueOnce(APP2) vi.mocked(ensureDeploymentIdsPresence).mockResolvedValue(identifiers) vi.mocked(loadApp).mockResolvedValue(app) + vi.mocked(link).mockResolvedValue(app.configuration) const developerPlatformClient = buildDeveloperPlatformClient({ async orgAndApps(_orgId: string) { return { @@ -1075,14 +1080,19 @@ describe('ensureDeploy Context', () => { test("throws an app not found error if the app with the Client ID doesn't exist", async () => { // Given - const app = testApp() + const app = testAppWithConfig() vi.mocked(getAppIdentifiers).mockReturnValue({app: APP1.apiKey}) vi.mocked(loadApp).mockResolvedValue(app) + vi.mocked(link).mockResolvedValue(app.configuration) + + const developerPlatformClient = testDeveloperPlatformClient({ + appFromId: vi.fn().mockRejectedValue(new AbortError("Couldn't find the app with Client ID key1")), + }) + vi.mocked(selectDeveloperPlatformClient).mockReturnValue(developerPlatformClient) + const opts = { ...deployOptions(app), - developerPlatformClient: testDeveloperPlatformClient({ - appFromId: vi.fn().mockRejectedValue(new AbortError("Couldn't find the app with Client ID key1")), - }), + developerPlatformClient, } // When @@ -1148,7 +1158,7 @@ describe('ensureDeploy Context', () => { }) test('load the app extension using the remote extensions specifications', async () => { // Given - const app = testApp() + const app = testAppWithConfig({config: {client_id: APP2.apiKey}}) const identifiers = { app: APP2.apiKey, extensions: {}, @@ -1164,6 +1174,8 @@ describe('ensureDeploy Context', () => { vi.mocked(ensureDeploymentIdsPresence).mockResolvedValue(identifiers) vi.mocked(loadApp).mockResolvedValue(appWithExtensions) vi.mocked(updateAppIdentifiers).mockResolvedValue(appWithExtensions) + vi.mocked(link).mockResolvedValue(app.configuration) + vi.mocked(selectDeveloperPlatformClient).mockReturnValue(buildDeveloperPlatformClient()) // When const got = await ensureDeployContext(deployOptions(app)) @@ -1195,6 +1207,7 @@ describe('ensureDeploy Context', () => { vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) vi.mocked(getAppConfigurationFileName).mockReturnValue('shopify.app.toml') vi.mocked(link).mockResolvedValue(app.configuration) + vi.mocked(selectDeveloperPlatformClient).mockReturnValue(buildDeveloperPlatformClient()) const writeAppConfigurationFileSpy = vi .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') @@ -1247,7 +1260,7 @@ describe('ensureDeploy Context', () => { vi.mocked(renderConfirmationPrompt).mockResolvedValue(false) vi.mocked(getAppConfigurationFileName).mockReturnValue('shopify.app.toml') vi.mocked(link).mockResolvedValue(app.configuration) - // vi.mocked(selectDeveloperPlatformClient).mockReturnValue(testDeveloperPlatformClient) + vi.mocked(selectDeveloperPlatformClient).mockReturnValue(buildDeveloperPlatformClient()) const writeAppConfigurationFileSpy = vi .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') .mockResolvedValue() @@ -1447,6 +1460,8 @@ describe('ensureDeploy Context', () => { vi.mocked(loadApp).mockResolvedValue(app) vi.mocked(renderConfirmationPrompt).mockResolvedValue(false) vi.mocked(getAppConfigurationFileName).mockReturnValue('shopify.app.toml') + vi.mocked(link).mockResolvedValue(app.configuration) + vi.mocked(selectDeveloperPlatformClient).mockReturnValue(buildDeveloperPlatformClient()) const writeAppConfigurationFileSpy = vi .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') .mockResolvedValue() diff --git a/packages/app/src/cli/services/context.ts b/packages/app/src/cli/services/context.ts index e53c2a2742..eb829f2757 100644 --- a/packages/app/src/cli/services/context.ts +++ b/packages/app/src/cli/services/context.ts @@ -429,7 +429,7 @@ export async function ensureDeployContext(options: DeployContextOptions): Promis let developerPlatformClient = options.developerPlatformClient // do the link here const performAppLink = await decideOnLinkStrategy(options.app.directory, options.reset, true) - const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient, performAppLink) + const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient, true, performAppLink) developerPlatformClient = remoteApp.developerPlatformClient ?? developerPlatformClient const specifications = await fetchSpecifications({developerPlatformClient, app: remoteApp}) From e7416aeaf73b76d7250edbdbfeb7a5dae249fd7a Mon Sep 17 00:00:00 2001 From: Mitch Dickinson Date: Mon, 29 Jul 2024 12:20:32 -0400 Subject: [PATCH 4/7] Tests working --- packages/app/src/cli/services/context.test.ts | 29 ++++++++++--------- packages/app/src/cli/services/context.ts | 6 +++- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/packages/app/src/cli/services/context.test.ts b/packages/app/src/cli/services/context.test.ts index 96dec7d8d2..1179be1e22 100644 --- a/packages/app/src/cli/services/context.test.ts +++ b/packages/app/src/cli/services/context.test.ts @@ -945,6 +945,9 @@ describe('ensureDeploy Context', () => { vi.mocked(loadApp).mockResolvedValue(app) vi.mocked(link).mockResolvedValue(app.configuration) vi.mocked(selectDeveloperPlatformClient).mockReturnValue(buildDeveloperPlatformClient()) + const writeAppConfigurationFileSpy = vi + .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') + .mockResolvedValue() // When const got = await ensureDeployContext(deployOptions(app)) @@ -1001,12 +1004,14 @@ describe('ensureDeploy Context', () => { vi.mocked(ensureDeploymentIdsPresence).mockResolvedValue(identifiers) vi.mocked(loadApp).mockResolvedValue(app) vi.mocked(link).mockResolvedValue(app.configuration) - vi.mocked(selectDeveloperPlatformClient).mockReturnValue(buildDeveloperPlatformClient()) + const writeAppConfigurationFileSpy = vi .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') .mockResolvedValue() const opts = deployOptions(app) + vi.mocked(selectDeveloperPlatformClient).mockReturnValue(opts.developerPlatformClient) + // When const got = await ensureDeployContext(opts) @@ -1028,7 +1033,7 @@ describe('ensureDeploy Context', () => { test('prompts the user to create or select an app and returns it with its id when the app has no extensions', async () => { // Given - const app = testAppWithConfig({config: {client_id: APP2.apiKey}}) + const app = testAppWithConfig({config: {client_id: APP1.apiKey}}) const identifiers = { app: APP1.apiKey, extensions: {}, @@ -1036,10 +1041,14 @@ describe('ensureDeploy Context', () => { extensionsNonUuidManaged: {}, } vi.mocked(getAppIdentifiers).mockReturnValue({app: undefined}) - vi.mocked(fetchAppDetailsFromApiKey).mockResolvedValueOnce(APP2) vi.mocked(ensureDeploymentIdsPresence).mockResolvedValue(identifiers) vi.mocked(loadApp).mockResolvedValue(app) - vi.mocked(link).mockResolvedValue(app.configuration) + vi.mocked(link).mockResolvedValue({...app.configuration, organization_id: ORG1.id}) + + const writeAppConfigurationFileSpy = vi + .spyOn(writeAppConfigurationFile, 'writeAppConfigurationFile') + .mockResolvedValue() + const developerPlatformClient = buildDeveloperPlatformClient({ async orgAndApps(_orgId: string) { return { @@ -1056,15 +1065,9 @@ describe('ensureDeploy Context', () => { const got = await ensureDeployContext(opts) // Then - expect(fetchOrganizations).toHaveBeenCalledOnce() - expect(selectOrCreateApp).toHaveBeenCalledWith( - app.name, - [APP1, APP2], - false, - ORG1, - opts.developerPlatformClient, - DEFAULT_SELECT_APP_OPTIONS, - ) + // TODO: try to assert exact arguments. + expect(link).toHaveBeenCalled() + expect(updateAppIdentifiers).toBeCalledWith({ app, identifiers, diff --git a/packages/app/src/cli/services/context.ts b/packages/app/src/cli/services/context.ts index eb829f2757..4d3396ed70 100644 --- a/packages/app/src/cli/services/context.ts +++ b/packages/app/src/cli/services/context.ts @@ -428,8 +428,10 @@ export async function ensureDeployContext(options: DeployContextOptions): Promis const {reset, force, noRelease} = options let developerPlatformClient = options.developerPlatformClient // do the link here + // MITCH: do a refactor to call getAppContext directly and not do this. const performAppLink = await decideOnLinkStrategy(options.app.directory, options.reset, true) const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient, true, performAppLink) + developerPlatformClient = remoteApp.developerPlatformClient ?? developerPlatformClient const specifications = await fetchSpecifications({developerPlatformClient, app: remoteApp}) @@ -632,6 +634,7 @@ export async function fetchOrCreateOrganizationApp( return remoteApp } +// TODO: Add an issue to potentially delete this method in a separate PR export async function fetchAppAndIdentifiers( options: { app: AppInterface @@ -661,6 +664,7 @@ export async function fetchAppAndIdentifiers( if (performAppLink) { envIdentifiers = {app: undefined, extensions: {}} reuseDevCache = false + // TODO: should we pass this in? We read the cached info outside this. const configuration = await link( { directory: app.directory, @@ -819,7 +823,7 @@ export async function getAppContext({ } } -async function decideOnLinkStrategy( +export async function decideOnLinkStrategy( directory: string, reset: boolean, enableLinkingPrompt: boolean, From 80d5514233f44d911d874d32125a5493c65b586a Mon Sep 17 00:00:00 2001 From: Mitch Dickinson Date: Mon, 29 Jul 2024 15:07:14 -0400 Subject: [PATCH 5/7] refactoring --- packages/app/src/cli/services/context.test.ts | 3 +- packages/app/src/cli/services/context.ts | 44 +++++-------------- 2 files changed, 13 insertions(+), 34 deletions(-) diff --git a/packages/app/src/cli/services/context.test.ts b/packages/app/src/cli/services/context.test.ts index 1179be1e22..ce54862226 100644 --- a/packages/app/src/cli/services/context.test.ts +++ b/packages/app/src/cli/services/context.test.ts @@ -1065,8 +1065,7 @@ describe('ensureDeploy Context', () => { const got = await ensureDeployContext(opts) // Then - // TODO: try to assert exact arguments. - expect(link).toHaveBeenCalled() + expect(link).toBeCalledWith({directory: app.directory}, false) expect(updateAppIdentifiers).toBeCalledWith({ app, diff --git a/packages/app/src/cli/services/context.ts b/packages/app/src/cli/services/context.ts index 4d3396ed70..d760861efa 100644 --- a/packages/app/src/cli/services/context.ts +++ b/packages/app/src/cli/services/context.ts @@ -429,8 +429,7 @@ export async function ensureDeployContext(options: DeployContextOptions): Promis let developerPlatformClient = options.developerPlatformClient // do the link here // MITCH: do a refactor to call getAppContext directly and not do this. - const performAppLink = await decideOnLinkStrategy(options.app.directory, options.reset, true) - const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient, true, performAppLink) + const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient, true, true) developerPlatformClient = remoteApp.developerPlatformClient ?? developerPlatformClient @@ -558,7 +557,7 @@ function includeConfigOnDeployPrompt(configPath: string): Promise { export async function ensureReleaseContext(options: ReleaseContextOptions): Promise { let developerPlatformClient = options.developerPlatformClient ?? selectDeveloperPlatformClient({configuration: options.app.configuration}) - const [remoteApp, envIdentifiers] = await fetchAppAndIdentifiers(options, developerPlatformClient) + const [remoteApp, envIdentifiers] = await fetchAppAndIdentifiers(options, developerPlatformClient, true, true) developerPlatformClient = remoteApp.developerPlatformClient ?? developerPlatformClient const identifiers: Identifiers = envIdentifiers as Identifiers @@ -643,7 +642,7 @@ export async function fetchAppAndIdentifiers( }, initialDeveloperPlatformClient: DeveloperPlatformClient, reuseFromDev = true, - performAppLink = false, + enableLinkingPrompt = false, ): Promise<[OrganizationApp, Partial]> { let developerPlatformClient = initialDeveloperPlatformClient const app = options.app @@ -651,28 +650,10 @@ export async function fetchAppAndIdentifiers( let envIdentifiers = getAppIdentifiers({app}, developerPlatformClient) let remoteApp: OrganizationApp | undefined - const previousCachedInfo = getCachedAppInfo(app.directory) - - /** - * TODO: - * - Clean up this refactor to the extent possible (and file an issue for further improvements) - * - Fixing existing tests - * - Add test that link is called on a new deploy - * - More tophatting. - * - Test dev flow with partners - */ - if (performAppLink) { + const configuration = await linkIfNecessary(app.directory, options.reset, enableLinkingPrompt) + if (configuration !== undefined) { envIdentifiers = {app: undefined, extensions: {}} reuseDevCache = false - // TODO: should we pass this in? We read the cached info outside this. - const configuration = await link( - { - directory: app.directory, - baseConfigName: previousCachedInfo?.configFile, - developerPlatformClient, - }, - false, - ) app.configuration = configuration developerPlatformClient = selectDeveloperPlatformClient({configuration}) } @@ -782,12 +763,9 @@ export async function getAppContext({ configName?: string enableLinkingPrompt?: boolean }): Promise { - const performAppLink = await decideOnLinkStrategy(directory, reset, enableLinkingPrompt) - let cachedInfo = getCachedAppInfo(directory) + await linkIfNecessary(directory, reset, enableLinkingPrompt) - if (performAppLink) { - await link({directory, baseConfigName: cachedInfo?.configFile}, false) - } + let cachedInfo = getCachedAppInfo(directory) const {configuration} = await loadAppConfiguration({ directory, @@ -823,11 +801,11 @@ export async function getAppContext({ } } -export async function decideOnLinkStrategy( +async function linkIfNecessary( directory: string, reset: boolean, enableLinkingPrompt: boolean, -): Promise { +): Promise { const previousCachedInfo = getCachedAppInfo(directory) if (reset) clearCachedAppInfo(directory) @@ -838,7 +816,9 @@ export async function decideOnLinkStrategy( previousCachedInfo?.configFile !== undefined && (await glob(joinPath(directory, 'shopify.app*.toml'))).length === 0 const performAppLink = enableLinkingPrompt && (firstTimeSetup || usingConfigAndResetting || usingConfigWithNoTomls) - return performAppLink + if (performAppLink) { + return link({directory, baseConfigName: previousCachedInfo?.configFile}, false) + } } /** From d2e97d2760509bdb763a79da8156f8b4d1b13e89 Mon Sep 17 00:00:00 2001 From: Mitch Dickinson Date: Mon, 29 Jul 2024 15:07:28 -0400 Subject: [PATCH 6/7] cleanup todo --- packages/app/src/cli/services/context.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/app/src/cli/services/context.ts b/packages/app/src/cli/services/context.ts index d760861efa..dffbc942a8 100644 --- a/packages/app/src/cli/services/context.ts +++ b/packages/app/src/cli/services/context.ts @@ -633,7 +633,6 @@ export async function fetchOrCreateOrganizationApp( return remoteApp } -// TODO: Add an issue to potentially delete this method in a separate PR export async function fetchAppAndIdentifiers( options: { app: AppInterface From 64750af57755e04e9e4c31606867d4107c227f62 Mon Sep 17 00:00:00 2001 From: Mitch Dickinson Date: Mon, 29 Jul 2024 15:13:11 -0400 Subject: [PATCH 7/7] cleanup --- packages/app/src/cli/services/context.test.ts | 2 +- packages/app/src/cli/services/context.ts | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/app/src/cli/services/context.test.ts b/packages/app/src/cli/services/context.test.ts index ce54862226..8e141d7424 100644 --- a/packages/app/src/cli/services/context.test.ts +++ b/packages/app/src/cli/services/context.test.ts @@ -929,7 +929,7 @@ api_version = "2023-04" }) }) -describe('ensureDeploy Context', () => { +describe('ensureDeployContext', () => { test("fetches the app from the partners' API and returns it alongside the id when identifiers are available locally and the app has no extensions", async () => { // Given const app = testAppWithConfig({config: {client_id: APP2.apiKey}}) diff --git a/packages/app/src/cli/services/context.ts b/packages/app/src/cli/services/context.ts index dffbc942a8..f789438d78 100644 --- a/packages/app/src/cli/services/context.ts +++ b/packages/app/src/cli/services/context.ts @@ -428,7 +428,6 @@ export async function ensureDeployContext(options: DeployContextOptions): Promis const {reset, force, noRelease} = options let developerPlatformClient = options.developerPlatformClient // do the link here - // MITCH: do a refactor to call getAppContext directly and not do this. const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient, true, true) developerPlatformClient = remoteApp.developerPlatformClient ?? developerPlatformClient