Merge pull request #5150 from Shopify/01-03-use_uid_instead_of_handle_to_identify_extensions_in_app-watcher

Use UID instead of handle to identify extensions in app-watcher

Author: isaacroldan

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

@@ -273,7 +273,7 @@ export interface AppInterface<
    */
   creationDefaultOptions(): AppCreationDefaultOptions
   manifest: () => Promise<JsonMapType>
-  removeExtension: (extensionHandle: string) => void
+  removeExtension: (extensionUid: string) => void
 }
 
 type AppConstructor<
@@ -441,8 +441,8 @@ export class App<
     }
   }
 
-  removeExtension(extensionHandle: string) {
-    this.realExtensions = this.realExtensions.filter((ext) => ext.handle !== extensionHandle)
+  removeExtension(extensionUid: string) {
+    this.realExtensions = this.realExtensions.filter((ext) => ext.uid !== extensionUid)
   }
 
   get includeConfigOnDeploy() {

packages/app/src/cli/models/extensions/extension-instance.ts

@@ -25,7 +25,7 @@ import {DeveloperPlatformClient} from '../../utilities/developer-platform-client
 import {AppConfigurationWithoutPath, CurrentAppConfiguration} from '../app/app.js'
 import {ok} from '@shopify/cli-kit/node/result'
 import {constantize, slugify} from '@shopify/cli-kit/common/string'
-import {hashString, nonRandomUUID, randomUUID} from '@shopify/cli-kit/node/crypto'
+import {hashString, nonRandomUUID} from '@shopify/cli-kit/node/crypto'
 import {partnersFqdn} from '@shopify/cli-kit/node/context/fqdn'
 import {joinPath, basename} from '@shopify/cli-kit/node/path'
 import {fileExists, touchFile, moveFile, writeFile, glob} from '@shopify/cli-kit/node/fs'
@@ -150,11 +150,12 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi
     this.directory = options.directory
     this.specification = options.specification
     this.handle = this.buildHandle()
-    this.devUUID = `dev-${nonRandomUUID(this.handle)}`
+    const uuidFromHandle = nonRandomUUID(this.handle)
+    this.devUUID = `dev-${uuidFromHandle}`
     this.localIdentifier = this.handle
     this.idEnvironmentVariableName = `SHOPIFY_${constantize(this.localIdentifier)}_ID`
     this.outputPath = this.directory
-    this.uid = this.configuration.uid ?? randomUUID()
+    this.uid = this.configuration.uid ?? uuidFromHandle
 
     if (this.features.includes('esbuild') || this.type === 'tax_calculation') {
       this.outputPath = joinPath(this.directory, 'dist', this.outputFileName)
@@ -369,7 +370,7 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi
   }
 
   getOutputFolderId(extensionId?: string) {
-    return extensionId ?? this.configuration.uid ?? this.handle
+    return extensionId ?? this.uid ?? this.handle
   }
 
   getOutputPathForDirectory(directory: string, extensionId?: string) {

packages/app/src/cli/services/dev/app-events/app-diffing.test.ts

@@ -2,8 +2,8 @@ import {appDiff} from './app-diffing.js'
 import {testApp, testAppConfigExtensions, testUIExtension} from '../../../models/app/app.test-data.js'
 import {describe, expect, test} from 'vitest'
 
-const extension1 = await testUIExtension({type: 'ui_extension', handle: 'h1', directory: '/extensions/ui_extension_1'})
-const extension2 = await testUIExtension({type: 'ui_extension', directory: '/extensions/ui_extension_2'})
+const extension1 = await testUIExtension({type: 'ui_extension', directory: '/extensions/ui_extension_1', uid: 'uid1'})
+const extension2 = await testUIExtension({type: 'ui_extension', directory: '/extensions/ui_extension_2', uid: 'uid2'})
 const posExtension = await testAppConfigExtensions()
 const posExtensionWithDifferentConfig = await testAppConfigExtensions(true)
 

packages/app/src/cli/services/dev/app-events/app-diffing.ts

@@ -20,20 +20,21 @@ interface AppExtensionsDiff {
  */
 export function appDiff(app: AppInterface, newApp: AppInterface, includeUpdated = true): AppExtensionsDiff {
   const oldExtensions = app.realExtensions
-  const oldExtensionsHandles = oldExtensions.map((ext) => ext.handle)
+  const oldExtensionsUids = oldExtensions.map((ext) => ext.uid)
   const newExtensions = newApp.realExtensions
-  const newExtensionsHandles = newExtensions.map((ext) => ext.handle)
+  const newExtensionsUids = newExtensions.map((ext) => ext.uid)
 
-  const createdExtensions = newExtensions.filter((ext) => !oldExtensionsHandles.includes(ext.handle))
-  const deletedExtensions = oldExtensions.filter((ext) => !newExtensionsHandles.includes(ext.handle))
+  const createdExtensions = newExtensions.filter((ext) => !oldExtensionsUids.includes(ext.uid))
+  const deletedExtensions = oldExtensions.filter((ext) => !newExtensionsUids.includes(ext.uid))
 
   let updatedExtensions
   if (includeUpdated) {
     updatedExtensions = newExtensions.filter((ext) => {
-      const oldConfig = oldExtensions.find((oldExt) => oldExt.handle === ext.handle)?.configuration
-      const newConfig = ext.configuration
-      if (oldConfig === undefined) return false
-      return JSON.stringify(oldConfig) !== JSON.stringify(newConfig)
+      const oldExtension = oldExtensions.find((oldExt) => oldExt.uid === ext.uid)
+      if (!oldExtension) return false
+      const configChanged = JSON.stringify(oldExtension.configuration) !== JSON.stringify(ext.configuration)
+      const extensionPathChanged = oldExtension.configurationPath !== ext.configurationPath
+      return configChanged || extensionPathChanged
     })
   }
 

packages/app/src/cli/services/dev/app-events/app-event-watcher-handler.ts

@@ -24,8 +24,9 @@ export async function handleWatcherEvents(
   const appReloadNeeded = events.some((event) => eventsThatRequireReload.includes(event.type))
   const otherEvents = events.filter((event) => !eventsThatRequireReload.includes(event.type))
 
-  let appEvent: AppEvent = {app, extensionEvents: [], path: events[0].path, startTime: events[0].startTime}
-  if (appReloadNeeded) appEvent = await ReloadAppHandler({event: events[0], app, options, extensions: []})
+  if (appReloadNeeded) return ReloadAppHandler({event: events[0], app, options, extensions: []})
+
+  const appEvent: AppEvent = {app, extensionEvents: [], path: events[0].path, startTime: events[0].startTime}
 
   for (const event of otherEvents) {
     const affectedExtensions = app.realExtensions.filter((ext) => ext.directory === event.extensionPath)
@@ -73,7 +74,7 @@ const handlers: {[key in WatcherEvent['type']]: Handler} = {
  */
 function ExtensionFolderDeletedHandler({event, app, extensions}: HandlerInput): AppEvent {
   const events = extensions.map((ext) => {
-    app.removeExtension(ext.handle)
+    app.removeExtension(ext.uid)
     return {type: EventType.Deleted, extension: ext}
   })
   return {app, extensionEvents: events, startTime: event.startTime, path: event.path}

packages/app/src/cli/services/dev/app-events/app-event-watcher.test.ts

@@ -17,15 +17,16 @@ import {AbortSignal, AbortController} from '@shopify/cli-kit/node/abort'
 import {flushPromises} from '@shopify/cli-kit/node/promises'
 import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs'
 import {joinPath} from '@shopify/cli-kit/node/path'
+import {nonRandomUUID} from '@shopify/cli-kit/node/crypto'
 import {Writable} from 'stream'
 
 vi.mock('../../../models/app/loader.js')
 vi.mock('./app-watcher-esbuild.js')
 
 // Extensions 1 and 1B simulate extensions defined in the same directory (same toml)
-const extension1 = await testUIExtension({type: 'ui_extension', handle: 'h1', directory: '/extensions/ui_extension_1'})
-const extension1B = await testUIExtension({type: 'ui_extension', handle: 'h2', directory: '/extensions/ui_extension_1'})
-const extension2 = await testUIExtension({type: 'ui_extension', directory: '/extensions/ui_extension_2'})
+const extension1 = await testUIExtension({type: 'ui_extension', directory: '/extensions/ui_extension_1', uid: 'uid1'})
+const extension1B = await testUIExtension({type: 'ui_extension', directory: '/extensions/ui_extension_1', uid: 'uid1B'})
+const extension2 = await testUIExtension({type: 'ui_extension', directory: '/extensions/ui_extension_2', uid: 'uid2'})
 const flowExtension = await testFlowActionExtension('/extensions/flow_action')
 const posExtension = await testAppConfigExtensions()
 const appAccessExtension = await testAppAccessConfigExtension()
@@ -35,14 +36,14 @@ const webhookExtension = await testSingleWebhookSubscriptionExtension()
 const extension1Updated = await testUIExtension({
   type: 'ui_extension',
   name: 'updated_name1',
-  handle: 'h1',
   directory: '/extensions/ui_extension_1',
+  uid: 'uid1',
 })
 const extension1BUpdated = await testUIExtension({
   type: 'ui_extension',
   name: 'updated_name1B',
-  handle: 'h2',
   directory: '/extensions/ui_extension_1',
+  uid: 'uid1B',
 })
 const posExtensionUpdated = await testAppConfigExtensions(true)
 
@@ -105,9 +106,7 @@ const testCases: TestCase[] = [
     },
     initialExtensions: [extension1, posExtension],
     finalExtensions: [extension1, extension2, posExtension],
-    extensionEvents: [
-      {type: EventType.Created, extension: extension2, buildResult: {status: 'ok', handle: 'test-ui-extension'}},
-    ],
+    extensionEvents: [{type: EventType.Created, extension: extension2, buildResult: {status: 'ok', uid: 'uid2'}}],
     needsAppReload: true,
   },
   {
@@ -120,7 +119,7 @@ const testCases: TestCase[] = [
     },
     initialExtensions: [extension1, extension2, posExtension],
     finalExtensions: [extension1, extension2, posExtension],
-    extensionEvents: [{type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', handle: 'h1'}}],
+    extensionEvents: [{type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', uid: 'uid1'}}],
   },
   {
     name: 'file_updated affecting a single extension',
@@ -132,7 +131,7 @@ const testCases: TestCase[] = [
     },
     initialExtensions: [extension1, extension2, posExtension],
     finalExtensions: [extension1, extension2, posExtension],
-    extensionEvents: [{type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', handle: 'h1'}}],
+    extensionEvents: [{type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', uid: 'uid1'}}],
   },
   {
     name: 'file_deleted affecting a single extension',
@@ -144,7 +143,7 @@ const testCases: TestCase[] = [
     },
     initialExtensions: [extension1, extension2, posExtension],
     finalExtensions: [extension1, extension2, posExtension],
-    extensionEvents: [{type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', handle: 'h1'}}],
+    extensionEvents: [{type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', uid: 'uid1'}}],
   },
   {
     name: 'file_created affecting a multiple extensions',
@@ -157,8 +156,8 @@ const testCases: TestCase[] = [
     initialExtensions: [extension1, extension1B, extension2, posExtension],
     finalExtensions: [extension1, extension1B, extension2, posExtension],
     extensionEvents: [
-      {type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', handle: 'h1'}},
-      {type: EventType.Updated, extension: extension1B, buildResult: {status: 'ok', handle: 'h2'}},
+      {type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', uid: 'uid1'}},
+      {type: EventType.Updated, extension: extension1B, buildResult: {status: 'ok', uid: 'uid1B'}},
     ],
   },
   {
@@ -172,8 +171,8 @@ const testCases: TestCase[] = [
     initialExtensions: [extension1, extension1B, extension2, posExtension],
     finalExtensions: [extension1, extension1B, extension2, posExtension],
     extensionEvents: [
-      {type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', handle: 'h1'}},
-      {type: EventType.Updated, extension: extension1B, buildResult: {status: 'ok', handle: 'h2'}},
+      {type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', uid: 'uid1'}},
+      {type: EventType.Updated, extension: extension1B, buildResult: {status: 'ok', uid: 'uid1B'}},
     ],
   },
   {
@@ -187,8 +186,8 @@ const testCases: TestCase[] = [
     initialExtensions: [extension1, extension1B, extension2, posExtension],
     finalExtensions: [extension1, extension1B, extension2, posExtension],
     extensionEvents: [
-      {type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', handle: 'h1'}},
-      {type: EventType.Updated, extension: extension1B, buildResult: {status: 'ok', handle: 'h2'}},
+      {type: EventType.Updated, extension: extension1, buildResult: {status: 'ok', uid: 'uid1'}},
+      {type: EventType.Updated, extension: extension1B, buildResult: {status: 'ok', uid: 'uid1B'}},
     ],
   },
   {
@@ -202,9 +201,17 @@ const testCases: TestCase[] = [
     initialExtensions: [extension1, extension2, posExtension, webhookExtension],
     finalExtensions: [extension1, extension2, posExtensionUpdated, appAccessExtension],
     extensionEvents: [
-      {type: EventType.Updated, extension: posExtensionUpdated, buildResult: {status: 'ok', handle: 'point-of-sale'}},
+      {
+        type: EventType.Updated,
+        extension: posExtensionUpdated,
+        buildResult: {status: 'ok', uid: nonRandomUUID('point-of-sale')},
+      },
       {type: EventType.Deleted, extension: webhookExtension},
-      {type: EventType.Created, extension: appAccessExtension, buildResult: {status: 'ok', handle: 'app-access'}},
+      {
+        type: EventType.Created,
+        extension: appAccessExtension,
+        buildResult: {status: 'ok', uid: nonRandomUUID('app-access')},
+      },
     ],
     needsAppReload: true,
   },
@@ -219,8 +226,8 @@ const testCases: TestCase[] = [
     initialExtensions: [extension1, extension1B, extension2],
     finalExtensions: [extension1Updated, extension1BUpdated, extension2],
     extensionEvents: [
-      {type: EventType.Updated, extension: extension1Updated, buildResult: {status: 'ok', handle: 'h1'}},
-      {type: EventType.Updated, extension: extension1BUpdated, buildResult: {status: 'ok', handle: 'h2'}},
+      {type: EventType.Updated, extension: extension1Updated, buildResult: {status: 'ok', uid: 'uid1'}},
+      {type: EventType.Updated, extension: extension1BUpdated, buildResult: {status: 'ok', uid: 'uid1B'}},
     ],
     needsAppReload: true,
   },
@@ -295,7 +302,7 @@ describe('app-event-watcher', () => {
           const initialEvents = app.realExtensions.map((eve) => ({
             type: EventType.Updated,
             extension: eve,
-            buildResult: {status: 'ok', handle: eve.handle},
+            buildResult: {status: 'ok', uid: eve.uid},
           }))
           expect(emitSpy).toHaveBeenCalledWith('ready', {
             app,
@@ -406,8 +413,9 @@ describe('app-event-watcher', () => {
 class MockESBuildContextManager extends ESBuildContextManager {
   contexts = {
     // The keys are the extension handles, the values are the ESBuild contexts mocked
-    h1: [{rebuild: vi.fn(), watch: vi.fn(), serve: vi.fn(), cancel: vi.fn(), dispose: vi.fn()}],
-    h2: [{rebuild: vi.fn(), watch: vi.fn(), serve: vi.fn(), cancel: vi.fn(), dispose: vi.fn()}],
+    uid1: [{rebuild: vi.fn(), watch: vi.fn(), serve: vi.fn(), cancel: vi.fn(), dispose: vi.fn()}],
+    uid1B: [{rebuild: vi.fn(), watch: vi.fn(), serve: vi.fn(), cancel: vi.fn(), dispose: vi.fn()}],
+    uid2: [{rebuild: vi.fn(), watch: vi.fn(), serve: vi.fn(), cancel: vi.fn(), dispose: vi.fn()}],
     'test-ui-extension': [{rebuild: vi.fn(), watch: vi.fn(), serve: vi.fn(), cancel: vi.fn(), dispose: vi.fn()}],
   }
 

packages/app/src/cli/services/dev/app-events/app-event-watcher.ts

@@ -83,7 +83,7 @@ export interface AppEvent {
   appWasReloaded?: boolean
 }
 
-type ExtensionBuildResult = {status: 'ok'; handle: string} | {status: 'error'; error: string; handle: string}
+type ExtensionBuildResult = {status: 'ok'; uid: string} | {status: 'error'; error: string; uid: string}
 
 /**
  * App event watcher will emit events when changes are detected in the file system.
@@ -230,12 +230,12 @@ export class AppEventWatcher extends EventEmitter {
       const ext = extEvent.extension
       return useConcurrentOutputContext({outputPrefix: ext.handle, stripAnsi: false}, async () => {
         try {
-          if (this.esbuildManager.contexts?.[ext.handle]?.length) {
+          if (this.esbuildManager.contexts?.[ext.uid]?.length) {
             await this.esbuildManager.rebuildContext(ext)
           } else {
             await this.buildExtension(ext)
           }
-          extEvent.buildResult = {status: 'ok', handle: ext.handle}
+          extEvent.buildResult = {status: 'ok', uid: ext.uid}
           // eslint-disable-next-line no-catch-all/no-catch-all, @typescript-eslint/no-explicit-any
         } catch (error: any) {
           // If there is an `errors` array, it's an esbuild error, format it and log it
@@ -249,7 +249,7 @@ export class AppEventWatcher extends EventEmitter {
           } else {
             this.options.stderr.write(error.message)
           }
-          extEvent.buildResult = {status: 'error', error: error.message, handle: ext.handle}
+          extEvent.buildResult = {status: 'error', error: error.message, uid: ext.uid}
         }
       })
     })

packages/app/src/cli/services/dev/app-events/app-watcher-esbuild.test.ts

@@ -12,8 +12,8 @@ vi.mock('@luckycatfactory/esbuild-graphql-loader', () => ({
   },
 }))
 
-const extension1 = await testUIExtension({type: 'ui_extension', handle: 'h1', directory: '/extensions/ui_extension_1'})
-const extension2 = await testUIExtension({type: 'ui_extension', directory: '/extensions/ui_extension_2'})
+const extension1 = await testUIExtension({type: 'ui_extension', directory: '/extensions/ui_extension_1', uid: 'uid1'})
+const extension2 = await testUIExtension({type: 'ui_extension', directory: '/extensions/ui_extension_2', uid: 'uid2'})
 
 describe('app-watcher-esbuild', () => {
   const options: DevAppWatcherOptions = {
@@ -31,8 +31,8 @@ describe('app-watcher-esbuild', () => {
     await manager.createContexts(extensions)
 
     // Then
-    expect(manager.contexts).toHaveProperty('h1')
-    expect(manager.contexts).toHaveProperty('test-ui-extension')
+    expect(manager.contexts).toHaveProperty('uid1')
+    expect(manager.contexts).toHaveProperty('uid2')
   })
 
   test('creating multiple contexts for the same extension', async () => {
@@ -44,6 +44,7 @@ describe('app-watcher-esbuild', () => {
     }
     const manager = new ESBuildContextManager(options)
     const extension = await testUIExtension({
+      uid: 'conditional-extension-uid',
       configuration: {
         ...extension2.configuration,
         handle: 'conditional-extension',
@@ -75,8 +76,8 @@ describe('app-watcher-esbuild', () => {
     await manager.createContexts([extension])
 
     // Then
-    expect(manager.contexts).toHaveProperty('conditional-extension')
-    expect(manager.contexts['conditional-extension']).toHaveLength(2)
+    expect(manager.contexts).toHaveProperty('conditional-extension-uid')
+    expect(manager.contexts['conditional-extension-uid']).toHaveLength(2)
   })
 
   test('deleting contexts', async () => {
@@ -89,8 +90,8 @@ describe('app-watcher-esbuild', () => {
     await manager.deleteContexts([extension1])
 
     // Then
-    expect(manager.contexts).not.toHaveProperty('h1')
-    expect(manager.contexts).toHaveProperty('test-ui-extension')
+    expect(manager.contexts).not.toHaveProperty('uid1')
+    expect(manager.contexts).toHaveProperty('uid2')
   })
 
   test('updating contexts with an app event', async () => {
@@ -112,22 +113,22 @@ describe('app-watcher-esbuild', () => {
     await manager.updateContexts(appEvent)
 
     // Then
-    expect(manager.contexts).toHaveProperty('h1')
-    expect(manager.contexts).not.toHaveProperty('test-ui-extension')
+    expect(manager.contexts).toHaveProperty('uid1')
+    expect(manager.contexts).not.toHaveProperty('uid2')
   })
 
   test('rebuilding contexts', async () => {
     // Given
     const manager = new ESBuildContextManager(options)
     await manager.createContexts([extension1])
-    const spyContext = vi.spyOn(manager.contexts.h1![0]!, 'rebuild').mockResolvedValue({} as any)
+    const spyContext = vi.spyOn(manager.contexts.uid1![0]!, 'rebuild').mockResolvedValue({} as any)
     const spyCopy = vi.spyOn(fs, 'copyFile').mockResolvedValue()
 
     // When
     await manager.rebuildContext(extension1)
 
     // Then
     expect(spyContext).toHaveBeenCalled()
-    expect(spyCopy).toHaveBeenCalledWith('/path/to/output/h1/dist', '/extensions/ui_extension_1/dist')
+    expect(spyCopy).toHaveBeenCalledWith('/path/to/output/uid1/dist', '/extensions/ui_extension_1/dist')
   })
 })