Skip to content

Fix how app name is set from the AppLoader #6561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gonzaloriestra merged 1 commit into from
Nov 4, 2025
Merged

Fix how app name is set from the AppLoader #6561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/happy-jobs-type.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@shopify/cli-kit': minor
'@shopify/app': minor
---

Use the handle to set the app version name, and the TOML name as a fallback
50 changes: 49 additions & 1 deletion packages/app/src/cli/models/app/loader.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ describe('load', () => {
}

const appConfiguration = `
name = "my_app"
Copy link
Contributor Author

@plvaldes plvaldes Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many changes in the test files are like this one: name is now required in test fixtures to be able to load the app

scopes = "read_products"
`
const linkedAppConfiguration = `
Expand Down Expand Up @@ -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 () => {
Expand Down
11 changes: 4 additions & 7 deletions packages/app/src/cli/models/app/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -343,7 +342,10 @@ 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 = (configuration as CurrentAppConfiguration).name
const configHandle: string | undefined = (configuration as CurrentAppConfiguration).handle
const name: string = this.previousApp?.name ?? configHandle ?? configName ?? ''
const nodeDependencies = this.previousApp?.nodeDependencies ?? (await getDependencies(packageJSONPath))
const packageManager = this.previousApp?.packageManager ?? (await getPackageManager(directory))
const usesWorkspaces = this.previousApp?.usesWorkspaces ?? (await appUsesWorkspaces(directory))
Expand Down Expand Up @@ -1108,11 +1110,6 @@ export async function loadHiddenConfig(
}
}

export async function loadAppName(appDirectory: string): Promise<string> {
Copy link
Contributor Author

@plvaldes plvaldes Nov 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will always have at least a name, so we don't need this fallback. Thus, this is dead code

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))
Expand Down
1 change: 1 addition & 0 deletions packages/app/src/cli/models/extensions/specifications/types/app_config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {WebhooksConfig} from './app_config_webhook.js'
*/
export interface AppConfigurationUsedByCli {
name: string
handle?: string
application_url: string
embedded: boolean
app_proxy?: {
Expand Down
39 changes: 30 additions & 9 deletions packages/app/src/cli/services/app-context.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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')),
},
}),
Expand Down Expand Up @@ -98,7 +101,10 @@ 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',
path: normalizePath(joinPath(tmp, 'shopify.app.stg.toml')),
},
},
configuration: {
client_id: 'test-api-key',
Expand Down Expand Up @@ -133,7 +139,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',
Expand Down Expand Up @@ -166,7 +174,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'

Expand All @@ -191,7 +201,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({
Expand All @@ -202,7 +214,10 @@ 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',
path: normalizePath(joinPath(tmp, 'shopify.app.toml')),
},
},
configuration: {
client_id: 'test-api-key',
Expand All @@ -229,7 +244,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
Expand All @@ -255,7 +272,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')

Expand All @@ -278,7 +297,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')

Expand Down