revert changes to urls.ts

Shopify · Oct 18, 2024 · 3e980d9 · 3e980d9
1 parent 8ceb4f8
commit 3e980d9
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 16 deletions.
diff --git 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
@@ -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
 }