From 8e2101004ad3237de88cfe3d488093e3fc31677f Mon Sep 17 00:00:00 2001 From: Melissa Luu Date: Tue, 5 May 2026 15:10:37 -0400 Subject: [PATCH] add-directories-to-watched-files --- .../extensions/extension-instance.test.ts | 75 +++++++++ .../models/extensions/extension-instance.ts | 15 ++ .../build/steps/include-assets-step.test.ts | 69 +++++++- .../build/steps/include-assets-step.ts | 7 +- .../dev/app-events/app-event-watcher.ts | 6 + .../dev/app-events/file-watcher.test.ts | 157 ++++++++++++++++++ .../services/dev/app-events/file-watcher.ts | 31 +++- 7 files changed, 353 insertions(+), 7 deletions(-) diff --git a/packages/app/src/cli/models/extensions/extension-instance.test.ts b/packages/app/src/cli/models/extensions/extension-instance.test.ts index 3fea6b4a055..aad31a8b9ec 100644 --- a/packages/app/src/cli/models/extensions/extension-instance.test.ts +++ b/packages/app/src/cli/models/extensions/extension-instance.test.ts @@ -717,6 +717,81 @@ describe('watchedFiles', async () => { vi.mocked(extractImportPathsRecursively).mockReset() }) }) + + test('includes additional watched paths registered via addWatchedPath', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const extensionInstance = await testUIExtension({ + directory: tmpDir, + entrySourceFilePath: joinPath(tmpDir, 'src', 'index.ts'), + }) + + const srcDir = joinPath(tmpDir, 'src') + await mkdir(srcDir) + await writeFile(joinPath(srcDir, 'index.ts'), 'code') + + vi.mocked(extractImportPathsRecursively).mockImplementation((filePath) => [filePath]) + + const assetsDir = joinPath(tmpDir, 'assets') + await mkdir(assetsDir) + extensionInstance.addWatchedPath(assetsDir) + + const watchedFiles = extensionInstance.watchedFiles() + + expect(watchedFiles).toContain(assetsDir) + + vi.mocked(extractImportPathsRecursively).mockReset() + }) + }) + + test('clearWatchedPaths removes all registered paths', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const extensionInstance = await testUIExtension({ + directory: tmpDir, + entrySourceFilePath: joinPath(tmpDir, 'src', 'index.ts'), + }) + + const srcDir = joinPath(tmpDir, 'src') + await mkdir(srcDir) + await writeFile(joinPath(srcDir, 'index.ts'), 'code') + + vi.mocked(extractImportPathsRecursively).mockImplementation((filePath) => [filePath]) + + extensionInstance.addWatchedPath(joinPath(tmpDir, 'assets')) + extensionInstance.clearWatchedPaths() + + const watchedFiles = extensionInstance.watchedFiles() + + expect(watchedFiles).not.toContain(joinPath(tmpDir, 'assets')) + + vi.mocked(extractImportPathsRecursively).mockReset() + }) + }) + + test('getWatchedPaths returns registered paths', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const extensionInstance = await testUIExtension({directory: tmpDir}) + + const assetsDir = joinPath(tmpDir, 'assets') + extensionInstance.addWatchedPath(assetsDir) + + const paths = extensionInstance.getWatchedPaths() + + expect(paths.has(assetsDir)).toBe(true) + expect(paths.size).toBe(1) + }) + }) + + test('addWatchedPath deduplicates identical paths', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const extensionInstance = await testUIExtension({directory: tmpDir}) + + const assetsDir = joinPath(tmpDir, 'assets') + extensionInstance.addWatchedPath(assetsDir) + extensionInstance.addWatchedPath(assetsDir) + + expect(extensionInstance.getWatchedPaths().size).toBe(1) + }) + }) }) describe('rescanImports', async () => { diff --git a/packages/app/src/cli/models/extensions/extension-instance.ts b/packages/app/src/cli/models/extensions/extension-instance.ts index e266cb54c0c..fbd7ec7b3c8 100644 --- a/packages/app/src/cli/models/extensions/extension-instance.ts +++ b/packages/app/src/cli/models/extensions/extension-instance.ts @@ -51,6 +51,7 @@ export class ExtensionInstance() get graphQLType() { return (this.specification.graphQLType ?? this.specification.identifier).toUpperCase() @@ -460,9 +461,23 @@ export class ExtensionInstance normalizePath(file)))] } + addWatchedPath(absolutePath: string): void { + this.additionalWatchedPaths.add(normalizePath(absolutePath)) + } + + getWatchedPaths(): ReadonlySet { + return this.additionalWatchedPaths + } + + clearWatchedPaths(): void { + this.additionalWatchedPaths.clear() + } + /** * Rescans imports for this extension and updates the cached import paths * Returns true if the imports changed diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts index e9decd6b02d..7dfc273af0f 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts @@ -18,7 +18,8 @@ describe('executeIncludeAssetsStep', () => { mockExtension = { directory: '/test/extension', outputPath: '/test/output/extension.js', - } as ExtensionInstance + addWatchedPath: vi.fn(), + } as unknown as ExtensionInstance mockContext = { extension: mockExtension, @@ -232,6 +233,72 @@ describe('executeIncludeAssetsStep', () => { expect(result.filesCopied).toBe(2) }) + test('registers directory source paths as watched paths on the extension', async () => { + // Given + const addWatchedPath = vi.fn() + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {static_root: 'public'}, + addWatchedPath, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['index.html', 'logo.png']) + + const step: LifecycleStep = { + id: 'copy-static', + name: 'Copy Static', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'static_root'}], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — directory source is registered as a watched path + expect(addWatchedPath).toHaveBeenCalledWith('/test/extension/public') + }) + + test('does not register file source paths as watched paths', async () => { + // Given + const addWatchedPath = vi.fn() + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {schema_path: 'src/schema.json'}, + addWatchedPath, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-schema', + name: 'Copy Schema', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'schema_path'}], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — file sources are not registered as watched paths + expect(addWatchedPath).not.toHaveBeenCalled() + }) + test('skips silently when configKey is absent from config', async () => { // Given — configuration has no static_root const contextWithoutConfig = { diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.ts b/packages/app/src/cli/services/build/steps/include-assets-step.ts index 1aba68d5d5d..335c25da33e 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.ts @@ -151,7 +151,12 @@ export async function executeIncludeAssetsStep( usedBasenames, preserveFilePaths: entry.preserveFilePaths, }) - result.pathMap.forEach((val, key) => aggregatedPathMap.set(key, val)) + result.pathMap.forEach((val, key) => { + aggregatedPathMap.set(key, val) + if (Array.isArray(val)) { + extension.addWatchedPath(joinPath(extension.directory, key)) + } + }) configKeyCount += result.filesCopied } diff --git a/packages/app/src/cli/services/dev/app-events/app-event-watcher.ts b/packages/app/src/cli/services/dev/app-events/app-event-watcher.ts index 5bda637f460..ff566ab190b 100644 --- a/packages/app/src/cli/services/dev/app-events/app-event-watcher.ts +++ b/packages/app/src/cli/services/dev/app-events/app-event-watcher.ts @@ -139,7 +139,13 @@ export class AppEventWatcher extends EventEmitter { const buildableEvents = appEvent.extensionEvents.filter((extEvent) => extEvent.type !== EventType.Deleted) // Build the created/updated extensions and update the extension events with the build result + const watchedPathsBefore = new Set(buildableEvents.flatMap((ev) => [...ev.extension.getWatchedPaths()])) await this.buildExtensions(buildableEvents) + const watchedPathsAfter = new Set(buildableEvents.flatMap((ev) => [...ev.extension.getWatchedPaths()])) + const watchedPathsChanged = + watchedPathsBefore.size !== watchedPathsAfter.size || + [...watchedPathsAfter].some((watchedPath) => !watchedPathsBefore.has(watchedPath)) + if (watchedPathsChanged) await this.fileWatcher?.start() // Generate the extension types after building the extensions so new imports are included // Skip if the app was reloaded, as generateExtensionTypes was already called during reload diff --git a/packages/app/src/cli/services/dev/app-events/file-watcher.test.ts b/packages/app/src/cli/services/dev/app-events/file-watcher.test.ts index 725604a59d1..feba2878cb1 100644 --- a/packages/app/src/cli/services/dev/app-events/file-watcher.test.ts +++ b/packages/app/src/cli/services/dev/app-events/file-watcher.test.ts @@ -810,6 +810,100 @@ describe('file-watcher events', () => { }) }) + describe('directory watched path handle resolution', () => { + test('resolves extension handle for files inside a watched directory', async () => { + // Given — extension with a watched directory (e.g., assets/) + const assetsDir = '/extensions/ui_extension_1/assets' + const ext = await testUIExtension({ + type: 'ui_extension', + handle: 'my-ext', + directory: '/extensions/ui_extension_1', + }) + // watchedFiles() returns individual files AND the directory path. + // shouldIgnoreEvent calls matchGlob against this list, so include + // a glob pattern for the directory so files inside it are not filtered out. + mockExtensionWatchedFiles(ext, ['/extensions/ui_extension_1/index.js', `${assetsDir}/**/*`, assetsDir]) + + const app = testAppLinked({ + allExtensions: [ext], + directory: '/', + configPath: '/shopify.app.toml', + configuration: { + ...DEFAULT_CONFIG, + extension_directories: ['/extensions'], + } as any, + }) + + let eventHandler: any + const mockWatcher = { + on: vi.fn((event: string, listener: any) => { + if (event === 'all') eventHandler = listener + return mockWatcher + }), + close: vi.fn(() => Promise.resolve()), + } + vi.spyOn(chokidar, 'watch').mockReturnValue(mockWatcher as any) + + const onChange = vi.fn() + const fileWatcher = new FileWatcher(app, outputOptions, 50) + fileWatcher.onChange(onChange) + await fileWatcher.start() + + // When — a file inside the watched directory changes + await eventHandler('change', `${assetsDir}/logo.png`) + await sleep(0.15) + + // Then — event is emitted with the correct extension handle + await vi.waitFor( + () => { + expect(onChange).toHaveBeenCalled() + const events = onChange.mock.calls.find((call) => call[0].length > 0)?.[0] + expect(events).toBeDefined() + expect(events[0].type).toBe('file_updated') + expect(events[0].extensionHandle).toBe('my-ext') + }, + {timeout: 1000, interval: 50}, + ) + }) + + test('does not pass watched directories to chokidar', async () => { + // Given — extension with both files and a directory in watchedFiles + const ext = await testUIExtension({ + type: 'ui_extension', + handle: 'my-ext', + directory: '/extensions/ui_extension_1', + }) + mockExtensionWatchedFiles(ext, ['/extensions/ui_extension_1/index.js', '/extensions/ui_extension_1/assets']) + + const app = testAppLinked({ + allExtensions: [ext], + directory: '/', + configPath: '/shopify.app.toml', + configuration: { + ...DEFAULT_CONFIG, + extension_directories: ['/extensions'], + } as any, + }) + + let watchedPaths: string[] = [] + vi.spyOn(chokidar, 'watch').mockImplementation((paths) => { + watchedPaths = paths as string[] + return { + on: vi.fn().mockReturnThis(), + close: vi.fn().mockResolvedValue(undefined), + } as any + }) + + // When + const fileWatcher = new FileWatcher(app, outputOptions) + await fileWatcher.start() + + // Then — files are passed to chokidar but directories are not + expect(watchedPaths).toContain('/extensions/ui_extension_1/index.js') + expect(watchedPaths).not.toContain('/extensions/ui_extension_1/assets') + }) + }) + describe('refreshWatchedFiles', () => { test('closes and recreates the watcher with updated paths', async () => { // Given @@ -844,4 +938,67 @@ describe('file-watcher events', () => { expect(watchedPaths[1]).toEqual(watchedPaths[0]) }) }) + + describe('file deletion events', () => { + test('file_deleted event is emitted even when the file is already gone from disk', async () => { + // Given: an extension with a watched file + const ext = await testUIExtension({ + type: 'ui_extension', + handle: 'del_ext', + directory: '/extensions/del_ext', + }) + const filePath = '/extensions/del_ext/assets/image.png' + // Simulate real behavior: watchedFiles() returns the file at start, + // but after deletion it no longer appears in the glob results + let watchedFilesResult: string[] = [filePath] + vi.spyOn(ext, 'watchedFiles').mockImplementation(() => watchedFilesResult) + + const testApp = testAppLinked({ + allExtensions: [ext], + directory: '/', + configPath: '/shopify.app.toml', + configuration: { + ...DEFAULT_CONFIG, + extension_directories: ['/extensions'], + } as any, + }) + + let eventHandler: any + const mockWatcher = { + on: vi.fn((event: string, listener: any) => { + if (event === 'all') eventHandler = listener + return mockWatcher + }), + close: vi.fn().mockResolvedValue(undefined), + } + vi.spyOn(chokidar, 'watch').mockReturnValue(mockWatcher as any) + vi.mocked(fileExistsSync).mockReturnValue(false) + + const fileWatcher = new FileWatcher(testApp, outputOptions, 50) + const onChange = vi.fn() + fileWatcher.onChange(onChange) + await fileWatcher.start() + await flushPromises() + + // Simulate the file being deleted from disk — watchedFiles() no longer returns it + watchedFilesResult = [] + await eventHandler('unlink', filePath, undefined) + await sleep(0.15) + + // Then: the file_deleted event should still be emitted + await vi.waitFor( + () => { + expect(onChange).toHaveBeenCalled() + const calls = onChange.mock.calls + const actualEvents = calls.find((call) => call[0].length > 0)?.[0] + expect(actualEvents).toBeDefined() + expect(actualEvents).toHaveLength(1) + expect(actualEvents[0].type).toBe('file_deleted') + expect(actualEvents[0].path).toBe(normalizePath(filePath)) + expect(actualEvents[0].extensionHandle).toBe('del_ext') + }, + {timeout: 1000, interval: 50}, + ) + }) + }) }) diff --git a/packages/app/src/cli/services/dev/app-events/file-watcher.ts b/packages/app/src/cli/services/dev/app-events/file-watcher.ts index 6b022e82cf2..0d36947f1e1 100644 --- a/packages/app/src/cli/services/dev/app-events/file-watcher.ts +++ b/packages/app/src/cli/services/dev/app-events/file-watcher.ts @@ -1,7 +1,7 @@ /* eslint-disable no-case-declarations */ import {AppLinkedInterface} from '../../../models/app/app.js' import {configurationFileNames} from '../../../constants.js' -import {dirname, joinPath, normalizePath, relativePath} from '@shopify/cli-kit/node/path' +import {basename, dirname, joinPath, normalizePath, relativePath} from '@shopify/cli-kit/node/path' import {FSWatcher} from 'chokidar' import {outputDebug} from '@shopify/cli-kit/node/output' import {AbortSignal} from '@shopify/cli-kit/node/abort' @@ -59,7 +59,7 @@ export class FileWatcher { private watcher?: FSWatcher private readonly debouncedEmit: () => void private readonly ignored: {[key: string]: ignore.Ignore | undefined} = {} - // Map of file paths to the extension handles that watch them + // Map of watched paths (files and directories) to the extension handles that own them private readonly extensionWatchedFiles = new Map>() constructor( @@ -159,18 +159,34 @@ export class FileWatcher { for (const {extension, watchedFiles} of extensionResults) { for (const file of watchedFiles) { const normalizedPath = normalizePath(file) - allFiles.add(normalizedPath) - // Track which extension handles watch this file + // Track which extension handles watch this path const handlesSet = this.extensionWatchedFiles.get(normalizedPath) ?? new Set() handlesSet.add(extension.handle) this.extensionWatchedFiles.set(normalizedPath, handlesSet) + + // Only pass files to chokidar — directories are already covered by + // extension directory watchers + if (basename(normalizedPath).includes('.')) { + allFiles.add(normalizedPath) + } } } return Array.from(allFiles) } + private getExtensionHandlesForFilePath(normalizedPath: string): Set | undefined { + const handles = this.extensionWatchedFiles.get(normalizedPath) + if (handles) return handles + for (const [watchedPath, pathHandles] of this.extensionWatchedFiles) { + if (normalizedPath.startsWith(`${watchedPath}/`)) { + return pathHandles + } + } + return undefined + } + /** * Emits the accumulated events and resets the current events list. * It also logs the number of events emitted and their paths for debugging purposes. @@ -223,6 +239,11 @@ export class FileWatcher { private shouldIgnoreEvent(event: WatcherEvent) { if (event.type === 'extension_folder_deleted' || event.type === 'extension_folder_created') return false + // For deleted files, skip the live watchPaths check — the file is already gone from disk + // so globSync won't find it. The file was already validated as belonging to the extension + // via the cached extensionWatchedFiles map earlier in the flow. + if (event.type === 'file_deleted') return false + const extension = event.extensionHandle ? this.app.realExtensions.find((ext) => ext.handle === event.extensionHandle) : undefined @@ -251,7 +272,7 @@ export class FileWatcher { if (isConfigAppPath) { this.handleEventForExtension(event, path, this.app.directory, startTime, false) } else { - const affectedHandles = this.extensionWatchedFiles.get(normalizedPath) + const affectedHandles = this.getExtensionHandlesForFilePath(normalizedPath) const isUnknownExtension = affectedHandles === undefined || affectedHandles.size === 0 if (isUnknownExtension && !isExtensionToml && !isConfigAppPath) {