Fix how app name is set from the AppLoader

Author: plvaldes

packages/app/src/cli/models/app/app.ts

@@ -48,6 +48,7 @@ export const LegacyAppSchema = zod
   .object({
     client_id: zod.number().optional(),
     name: zod.string().optional(),
+    handle: zod.string().optional(),
     scopes: zod
       .string()
       .transform((scopes) => normalizeDelimitedString(scopes) ?? '')
@@ -80,14 +81,16 @@ function fixSingleWildcards(value: string[] | undefined) {
  */
 export const AppSchema = zod.object({
   client_id: zod.string(),
+  extension_directories: ExtensionDirectoriesSchema,
+  name: zod.string().optional(),
+  handle: zod.string().optional(),
   build: zod
     .object({
       automatically_update_urls_on_dev: zod.boolean().optional(),
       dev_store_url: zod.string().optional(),
       include_config_on_deploy: zod.boolean().optional(),
     })
     .optional(),
-  extension_directories: ExtensionDirectoriesSchema,
   web_directories: zod.array(zod.string()).optional(),
 })
 
@@ -168,7 +171,7 @@ export function isLegacyAppSchema(item: AppConfiguration): item is LegacyAppConf
  */
 export function isCurrentAppSchema(item: AppConfiguration): item is CurrentAppConfiguration {
   const {path, ...rest} = item
-  return isType(AppSchema.nonstrict(), rest)
+  return isType(AppSchema.passthrough(), rest)
 }
 
 /**

packages/app/src/cli/models/app/loader.test.ts

@@ -64,6 +64,7 @@ describe('load', () => {
   }
 
   const appConfiguration = `
+name = "my_app"
 scopes = "read_products"
 `
   const linkedAppConfiguration = `
@@ -264,7 +265,54 @@ wrong = "property"
     const app = await loadApp({directory: tmpDir, specifications: [], userProvidedConfigName: undefined})
 
     // Then
-    expect(app.name).toBe('my_app')
+    expect(app.name).toBe('for-testing')
+  })
+
+  test('uses handle from configuration as app name when present', async () => {
+    // Given
+    const appConfiguration = `
+name = "display-name"
+handle = "app-handle"
+client_id = "1234567890"
+application_url = "https://example.com/lala"
+embedded = true
+
+[webhooks]
+api_version = "2023-07"
+
+[auth]
+redirect_urls = [ "https://example.com/api/auth" ]
+`
+    await writeConfig(appConfiguration)
+
+    // When
+    const app = await loadTestingApp()
+
+    // Then
+    expect(app.name).toBe('app-handle')
+  })
+
+  test('uses name from configuration when handle is not present', async () => {
+    // Given
+    const appConfiguration = `
+name = "config-name"
+client_id = "1234567890"
+application_url = "https://example.com/lala"
+embedded = true
+
+[webhooks]
+api_version = "2023-07"
+
+[auth]
+redirect_urls = [ "https://example.com/api/auth" ]
+`
+    await writeConfig(appConfiguration)
+
+    // When
+    const app = await loadTestingApp()
+
+    // Then
+    expect(app.name).toBe('config-name')
   })
 
   test('defaults to npm as the package manager when the configuration is valid', async () => {
@@ -618,6 +666,7 @@ wrong = "property"
   test('loads the app with custom located web blocks', async () => {
     // Given
     const {webDirectory} = await writeConfig(`
+    name = "my_app"
     scopes = ""
     web_directories = ["must_be_here"]
     `)
@@ -633,6 +682,7 @@ wrong = "property"
   test('loads the app with custom located web blocks, only checks given directory', async () => {
     // Given
     const {webDirectory} = await writeConfig(`
+    name = "my_app"
     scopes = ""
     web_directories = ["must_be_here"]
     `)
@@ -700,6 +750,7 @@ wrong = "property"
   test('loads the app when it has a extension with a valid configuration using a supported extension type and in a non-conventional directory configured in the app configuration file', async () => {
     // Given
     await writeConfig(`
+    name = "my_app"
     scopes = ""
     extension_directories = ["custom_extension"]
     `)

packages/app/src/cli/models/app/loader.ts

@@ -42,7 +42,6 @@ import {readAndParseDotEnv, DotEnvFile} from '@shopify/cli-kit/node/dot-env'
 import {
   getDependencies,
   getPackageManager,
-  getPackageName,
   usesWorkspaces as appUsesWorkspaces,
 } from '@shopify/cli-kit/node/node-package-manager'
 import {resolveFramework} from '@shopify/cli-kit/node/framework'
@@ -343,7 +342,18 @@ class AppLoader<TConfig extends AppConfiguration, TModuleSpec extends ExtensionS
     const packageJSONPath = joinPath(directory, 'package.json')
 
     // These don't need to be processed again if the app is being reloaded
-    const name = this.previousApp?.name ?? (await loadAppName(directory))
+    // name is required by BrandingSchema, so it must be present in the configuration
+    const configName = 'name' in configuration ? configuration.name : undefined
+    const name = this.previousApp?.name ?? configuration.handle ?? configName ?? ''
+    if (!name) {
+      this.abortOrReport(
+        outputContent`App name is missing from the configuration. Please add a ${outputToken.yellow(
+          'name',
+        )} field to your ${outputToken.path(configuration.path)}`,
+        '',
+        configuration.path,
+      )
+    }
     const nodeDependencies = this.previousApp?.nodeDependencies ?? (await getDependencies(packageJSONPath))
     const packageManager = this.previousApp?.packageManager ?? (await getPackageManager(directory))
     const usesWorkspaces = this.previousApp?.usesWorkspaces ?? (await appUsesWorkspaces(directory))
@@ -1108,11 +1118,6 @@ export async function loadHiddenConfig(
   }
 }
 
-export async function loadAppName(appDirectory: string): Promise<string> {
-  const packageJSONPath = joinPath(appDirectory, 'package.json')
-  return (await getPackageName(packageJSONPath)) ?? basename(appDirectory)
-}
-
 async function getProjectType(webs: Web[]): Promise<'node' | 'php' | 'ruby' | 'frontend' | undefined> {
   const backendWebs = webs.filter((web) => isWebType(web, WebType.Backend))
   const frontendWebs = webs.filter((web) => isWebType(web, WebType.Frontend))

packages/app/src/cli/services/app-context.test.ts

@@ -49,7 +49,9 @@ describe('linkedAppContext', () => {
   test('returns linked app context when app is already linked', async () => {
     await inTemporaryDirectory(async (tmp) => {
       // Given
-      const content = `client_id="test-api-key"`
+      const content = `
+name = "test-app"
+client_id="test-api-key"`
       await writeAppConfig(tmp, content)
 
       // When
@@ -65,6 +67,7 @@ describe('linkedAppContext', () => {
         app: expect.objectContaining({
           configuration: {
             client_id: 'test-api-key',
+            name: 'test-app',
             path: normalizePath(joinPath(tmp, 'shopify.app.toml')),
           },
         }),
@@ -98,7 +101,11 @@ describe('linkedAppContext', () => {
           configurationPath: `${tmp}/shopify.app.stg.toml`,
           configSource: 'cached',
           configurationFileName: 'shopify.app.stg.toml',
-          basicConfiguration: {client_id: 'test-api-key', path: normalizePath(joinPath(tmp, 'shopify.app.stg.toml'))},
+          basicConfiguration: {
+            client_id: 'test-api-key',
+            name: 'test-app',
+            path: normalizePath(joinPath(tmp, 'shopify.app.stg.toml')),
+          },
         },
         configuration: {
           client_id: 'test-api-key',
@@ -133,7 +140,9 @@ describe('linkedAppContext', () => {
     await inTemporaryDirectory(async (tmp) => {
       // Given
       vi.mocked(appFromIdentifiers).mockResolvedValue({...mockRemoteApp, apiKey: 'test-api-key-new'})
-      const content = `client_id="test-api-key-new"`
+      const content = `
+name = "test-app"
+client_id="test-api-key-new"`
       await writeAppConfig(tmp, content)
       localStorage.setCachedAppInfo({
         appId: 'test-api-key-old',
@@ -166,7 +175,9 @@ describe('linkedAppContext', () => {
   test('uses provided clientId when available and updates the app configuration', async () => {
     await inTemporaryDirectory(async (tmp) => {
       // Given
-      const content = `client_id="test-api-key"`
+      const content = `
+name = "test-app"
+client_id="test-api-key"`
       await writeAppConfig(tmp, content)
       const newClientId = 'new-api-key'
 
@@ -191,7 +202,9 @@ describe('linkedAppContext', () => {
   test('resets app when there is a valid toml but reset option is true', async () => {
     await inTemporaryDirectory(async (tmp) => {
       // Given
-      const content = `client_id="test-api-key"`
+      const content = `
+name = "test-app"
+client_id="test-api-key"`
       await writeAppConfig(tmp, content)
 
       vi.mocked(link).mockResolvedValue({
@@ -202,7 +215,11 @@ describe('linkedAppContext', () => {
           configurationPath: `${tmp}/shopify.app.toml`,
           configSource: 'cached',
           configurationFileName: 'shopify.app.toml',
-          basicConfiguration: {client_id: 'test-api-key', path: normalizePath(joinPath(tmp, 'shopify.app.toml'))},
+          basicConfiguration: {
+            client_id: 'test-api-key',
+            name: 'test-app',
+            path: normalizePath(joinPath(tmp, 'shopify.app.toml')),
+          },
         },
         configuration: {
           client_id: 'test-api-key',
@@ -229,7 +246,9 @@ describe('linkedAppContext', () => {
   test('logs metadata', async () => {
     await inTemporaryDirectory(async (tmp) => {
       // Given
-      const content = `client_id="test-api-key"`
+      const content = `
+name = "test-app"
+client_id="test-api-key"`
       await writeAppConfig(tmp, content)
 
       // When
@@ -255,7 +274,9 @@ describe('linkedAppContext', () => {
   test('uses unsafeReportMode when provided', async () => {
     await inTemporaryDirectory(async (tmp) => {
       // Given
-      const content = `client_id="test-api-key"`
+      const content = `
+name = "test-app"
+client_id="test-api-key"`
       await writeAppConfig(tmp, content)
       const loadSpy = vi.spyOn(loader, 'loadAppUsingConfigurationState')
 
@@ -278,7 +299,9 @@ describe('linkedAppContext', () => {
   test('does not use unsafeReportMode when not provided', async () => {
     await inTemporaryDirectory(async (tmp) => {
       // Given
-      const content = `client_id="test-api-key"`
+      const content = `
+name = "test-app"
+client_id="test-api-key"`
       await writeAppConfig(tmp, content)
       const loadSpy = vi.spyOn(loader, 'loadAppUsingConfigurationState')
 

packages/app/src/cli/services/context/breakdown-extensions.test.ts

@@ -874,7 +874,7 @@ describe('configExtensionsIdentifiersBreakdown', () => {
 
       // Then
       expect(result).toEqual({
-        existingFieldNames: ['name', 'application_url', 'embedded', 'pos', 'app_proxy', 'webhooks'],
+        existingFieldNames: ['application_url', 'embedded', 'pos', 'app_proxy', 'webhooks'],
         existingUpdatedFieldNames: [],
         newFieldNames: [],
         deletedFieldNames: [],
@@ -965,7 +965,7 @@ describe('configExtensionsIdentifiersBreakdown', () => {
 
       // Then
       expect(result).toEqual({
-        existingFieldNames: ['name', 'application_url', 'embedded', 'webhooks'],
+        existingFieldNames: ['application_url', 'embedded', 'webhooks'],
         existingUpdatedFieldNames: [],
         newFieldNames: [],
         deletedFieldNames: [],
@@ -1055,7 +1055,7 @@ describe('configExtensionsIdentifiersBreakdown', () => {
 
       // Then
       expect(result).toEqual({
-        existingFieldNames: ['name', 'webhooks'],
+        existingFieldNames: ['webhooks'],
         existingUpdatedFieldNames: ['application_url', 'embedded'],
         newFieldNames: [],
         deletedFieldNames: [],
@@ -1114,7 +1114,7 @@ describe('configExtensionsIdentifiersBreakdown', () => {
       expect(result).toEqual({
         existingFieldNames: ['application_url', 'embedded'],
         existingUpdatedFieldNames: [],
-        newFieldNames: ['name', 'webhooks', 'pos'],
+        newFieldNames: ['webhooks', 'pos'],
         deletedFieldNames: [],
       })
     })
@@ -1217,7 +1217,7 @@ describe('configExtensionsIdentifiersBreakdown', () => {
 
       // Then
       expect(result).toEqual({
-        existingFieldNames: ['name', 'application_url', 'embedded', 'webhooks'],
+        existingFieldNames: ['application_url', 'embedded', 'webhooks'],
         existingUpdatedFieldNames: [],
         newFieldNames: [],
         deletedFieldNames: ['pos'],
@@ -1808,7 +1808,7 @@ describe('configExtensionsIdentifiersBreakdown', () => {
       expect(result).toEqual({
         existingFieldNames: ['webhooks', 'application_url'],
         existingUpdatedFieldNames: [],
-        newFieldNames: ['name', 'embedded'],
+        newFieldNames: ['embedded'],
         deletedFieldNames: [],
       })
     })
@@ -1847,7 +1847,7 @@ describe('configExtensionsIdentifiersBreakdown', () => {
       expect(result).toEqual({
         existingFieldNames: [],
         existingUpdatedFieldNames: [],
-        newFieldNames: ['name', 'application_url', 'embedded', 'webhooks'],
+        newFieldNames: ['application_url', 'embedded', 'webhooks'],
         deletedFieldNames: [],
       })
     })