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 5a808958df..72886e6543 100644 --- a/packages/app/src/cli/models/extensions/extension-instance.test.ts +++ b/packages/app/src/cli/models/extensions/extension-instance.test.ts @@ -694,3 +694,31 @@ describe('rescanImports', async () => { }) }) }) + +describe('SHOPIFY_CLI_DISABLE_IMPORT_SCANNING', () => { + test('skips import scanning when env var is set', 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'), 'import "../shared"') + + vi.mocked(extractImportPathsRecursively).mockReset() + vi.mocked(extractImportPathsRecursively).mockReturnValue(['/some/external/file.ts']) + + process.env.SHOPIFY_CLI_DISABLE_IMPORT_SCANNING = '1' + try { + const watched = extensionInstance.watchedFiles() + expect(extractImportPathsRecursively).not.toHaveBeenCalled() + expect(watched.some((file) => file.includes('external'))).toBe(false) + } finally { + delete process.env.SHOPIFY_CLI_DISABLE_IMPORT_SCANNING + vi.mocked(extractImportPathsRecursively).mockReset() + } + }) + }) +}) diff --git a/packages/app/src/cli/models/extensions/extension-instance.ts b/packages/app/src/cli/models/extensions/extension-instance.ts index 1732badeae..cd3a89c914 100644 --- a/packages/app/src/cli/models/extensions/extension-instance.ts +++ b/packages/app/src/cli/models/extensions/extension-instance.ts @@ -31,7 +31,13 @@ import {joinPath, basename, normalizePath, resolvePath} from '@shopify/cli-kit/n import {fileExists, touchFile, moveFile, writeFile, glob, copyFile, globSync} from '@shopify/cli-kit/node/fs' import {getPathValue} from '@shopify/cli-kit/common/object' import {outputDebug} from '@shopify/cli-kit/node/output' -import {extractJSImports, extractImportPathsRecursively} from '@shopify/cli-kit/node/import-extractor' +import { + extractJSImports, + extractImportPathsRecursively, + clearImportPathsCache, + getImportScanningCacheStats, +} from '@shopify/cli-kit/node/import-extractor' +import {isTruthy} from '@shopify/cli-kit/node/context/utilities' import {uniq} from '@shopify/cli-kit/common/array' export const CONFIG_EXTENSION_IDS: string[] = [ @@ -516,6 +522,7 @@ export class ExtensionInstance { const oldImportPaths = this.cachedImportPaths this.cachedImportPaths = undefined + clearImportPathsCache() this.scanImports() return oldImportPaths !== this.cachedImportPaths } @@ -530,13 +537,26 @@ export class ExtensionInstance { + const startTime = performance.now() + const entryFiles = this.devSessionDefaultWatchPaths() + + const imports = entryFiles.flatMap((entryFile) => { return extractImportPathsRecursively(entryFile).map((importPath) => normalizePath(resolvePath(importPath))) }) - // Cache and return unique paths + this.cachedImportPaths = uniq(imports) ?? [] - outputDebug(`Found ${this.cachedImportPaths.length} external imports (recursively) for extension ${this.handle}`) + const elapsed = Math.round(performance.now() - startTime) + const cacheStats = getImportScanningCacheStats() + const cacheInfo = cacheStats ? ` (cache: ${cacheStats.directImports} parsed, ${cacheStats.fileExists} stats)` : '' + outputDebug( + `Import scan for "${this.handle}": ${entryFiles.length} entries, ${this.cachedImportPaths.length} files, ${elapsed}ms${cacheInfo}`, + ) return this.cachedImportPaths // eslint-disable-next-line no-catch-all/no-catch-all } catch (error) { diff --git a/packages/cli-kit/src/private/node/constants.ts b/packages/cli-kit/src/private/node/constants.ts index 3672b8b4c2..652d60cd96 100644 --- a/packages/cli-kit/src/private/node/constants.ts +++ b/packages/cli-kit/src/private/node/constants.ts @@ -46,6 +46,7 @@ export const environmentVariables = { neverUsePartnersApi: 'SHOPIFY_CLI_NEVER_USE_PARTNERS_API', skipNetworkLevelRetry: 'SHOPIFY_CLI_SKIP_NETWORK_LEVEL_RETRY', maxRequestTimeForNetworkCalls: 'SHOPIFY_CLI_MAX_REQUEST_TIME_FOR_NETWORK_CALLS', + disableImportScanning: 'SHOPIFY_CLI_DISABLE_IMPORT_SCANNING', } export const defaultThemeKitAccessDomain = 'theme-kit-access.shopifyapps.com' diff --git a/packages/cli-kit/src/public/node/import-extractor.test.ts b/packages/cli-kit/src/public/node/import-extractor.test.ts index e707ad5693..5a9b1ba8c0 100644 --- a/packages/cli-kit/src/public/node/import-extractor.test.ts +++ b/packages/cli-kit/src/public/node/import-extractor.test.ts @@ -1,7 +1,16 @@ import {inTemporaryDirectory, mkdir, writeFile} from './fs.js' import {joinPath} from './path.js' -import {extractImportPaths, extractImportPathsRecursively} from './import-extractor.js' -import {describe, test, expect} from 'vitest' +import { + extractImportPaths, + extractImportPathsRecursively, + clearImportPathsCache, + getImportScanningCacheStats, +} from './import-extractor.js' +import {describe, test, expect, beforeEach} from 'vitest' + +beforeEach(() => { + clearImportPathsCache() +}) describe('extractImportPaths', () => { describe('JavaScript imports', () => { @@ -610,3 +619,79 @@ describe('extractImportPathsRecursively', () => { }) }) }) + +describe('clearImportPathsCache', () => { + test('picks up file changes after cache is cleared', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const mainFile = joinPath(tmpDir, 'main.js') + const utilsFile = joinPath(tmpDir, 'utils.js') + + await writeFile(utilsFile, 'export const foo = "bar"') + await writeFile(mainFile, `import { foo } from './utils.js'`) + + const firstResult = extractImportPaths(mainFile) + expect(firstResult).toContain(utilsFile) + + // Modify the file to add a new import + const helpersFile = joinPath(tmpDir, 'helpers.js') + await writeFile(helpersFile, 'export const helper = () => {}') + await writeFile(mainFile, `import { foo } from './utils.js'\nimport { helper } from './helpers.js'`) + + // Without clearing, we still get cached results + const cachedResult = extractImportPaths(mainFile) + expect(cachedResult).toHaveLength(1) + + // After clearing, new imports are picked up + clearImportPathsCache() + const freshResult = extractImportPaths(mainFile) + expect(freshResult).toContain(utilsFile) + expect(freshResult).toContain(helpersFile) + expect(freshResult).toHaveLength(2) + }) + }) + + test('clears all caches including filesystem stat caches', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const mainFile = joinPath(tmpDir, 'main.js') + const utilsFile = joinPath(tmpDir, 'utils.js') + + await writeFile(utilsFile, 'export const foo = "bar"') + await writeFile(mainFile, `import { foo } from './utils.js'`) + + extractImportPaths(mainFile) + const statsAfterScan = getImportScanningCacheStats() + expect(statsAfterScan.directImports).toBeGreaterThan(0) + expect(statsAfterScan.fileExists).toBeGreaterThan(0) + + clearImportPathsCache() + const statsAfterClear = getImportScanningCacheStats() + expect(statsAfterClear.directImports).toBe(0) + expect(statsAfterClear.fileExists).toBe(0) + expect(statsAfterClear.isDir).toBe(0) + }) + }) +}) + +describe('getImportScanningCacheStats', () => { + test('tracks cache sizes across multiple scans', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const file1 = joinPath(tmpDir, 'file1.js') + const file2 = joinPath(tmpDir, 'file2.js') + const shared = joinPath(tmpDir, 'shared.js') + + await writeFile(shared, 'export const x = 1') + await writeFile(file1, `import { x } from './shared.js'`) + await writeFile(file2, `import { x } from './shared.js'`) + + extractImportPathsRecursively(file1) + const statsAfterFirst = getImportScanningCacheStats() + + extractImportPathsRecursively(file2) + const statsAfterSecond = getImportScanningCacheStats() + + // Second scan should grow the directImports cache (file2 is new) + // but shared.js stat results are reused from the first scan + expect(statsAfterSecond.directImports).toBeGreaterThanOrEqual(statsAfterFirst.directImports) + }) + }) +}) diff --git a/packages/cli-kit/src/public/node/import-extractor.ts b/packages/cli-kit/src/public/node/import-extractor.ts index a08c3501d7..637569d944 100644 --- a/packages/cli-kit/src/public/node/import-extractor.ts +++ b/packages/cli-kit/src/public/node/import-extractor.ts @@ -1,17 +1,58 @@ import {readFileSync, fileExistsSync, isDirectorySync} from './fs.js' import {dirname, joinPath} from './path.js' +// Caches direct import results per file path to avoid redundant file reads and parsing +// when multiple extensions import the same shared code. +const directImportsCache = new Map() + +// Caches filesystem stat results to avoid redundant synchronous I/O. +// Each stat call also triggers outputDebug overhead, so caching here +// avoids both the kernel round-trip and the debug string construction. +const fileExistsCache = new Map() +const isDirCache = new Map() + +function cachedFileExists(path: string): boolean { + const cached = fileExistsCache.get(path) + if (cached !== undefined) return cached + const result = fileExistsSync(path) + fileExistsCache.set(path, result) + return result +} + +function cachedIsDir(path: string): boolean { + const cached = isDirCache.get(path) + if (cached !== undefined) return cached + const result = isDirectorySync(path) + isDirCache.set(path, result) + return result +} + +/** + * Clears all import-scanning caches (direct imports, recursive results, and filesystem stats). + * Should be called when watched files change so that rescanning picks up updated imports. + */ +export function clearImportPathsCache(): void { + directImportsCache.clear() + fileExistsCache.clear() + isDirCache.clear() +} + /** * Extracts import paths from a source file. * Supports JavaScript, TypeScript, and Rust files. + * Results are cached per file path to avoid redundant I/O. * * @param filePath - Path to the file to analyze. * @returns Array of absolute paths to imported files. */ export function extractImportPaths(filePath: string): string[] { + const cached = directImportsCache.get(filePath) + if (cached) return cached + const content = readFileSync(filePath).toString() const ext = filePath.substring(filePath.lastIndexOf('.')) + let result: string[] switch (ext) { case '.js': case '.mjs': @@ -19,12 +60,17 @@ export function extractImportPaths(filePath: string): string[] { case '.ts': case '.tsx': case '.jsx': - return extractJSLikeImports(content, filePath) + result = extractJSLikeImports(content, filePath) + break case '.rs': - return extractRustImports(content, filePath) + result = extractRustImports(content, filePath) + break default: - return [] + result = [] } + + directImportsCache.set(filePath, result) + return result } /** @@ -38,40 +84,46 @@ export function extractImportPaths(filePath: string): string[] { * @throws If an unexpected error occurs while processing files (not including ENOENT file not found errors). */ export function extractImportPathsRecursively(filePath: string, visited: Set = new Set()): string[] { - // Avoid processing the same file twice (handles circular dependencies) if (visited.has(filePath)) { return [] } visited.add(filePath) - // Get direct imports from this file const directImports = extractImportPaths(filePath) const allImports = [filePath, ...directImports] - // Recursively process each imported file for (const importedFile of directImports) { try { - // Only process files that exist and are not directories - // Note: resolveJSImport already resolves directory imports to their index files - if (fileExistsSync(importedFile) && !isDirectorySync(importedFile)) { + if (cachedFileExists(importedFile) && !cachedIsDir(importedFile)) { const nestedImports = extractImportPathsRecursively(importedFile, visited) allImports.push(...nestedImports) } } catch (error) { - // Rethrow unexpected errors after checking for expected file read errors if (error instanceof Error && error.message.includes('ENOENT')) { - // Skip files that don't exist or can't be read continue } throw error } } - // Return unique list of imports return [...new Set(allImports)] } +/** + * Returns diagnostic information about the import scanning caches. + * Useful for debugging performance issues with --verbose. + * + * @returns Cache size stats for directImports, fileExists, and isDir. + */ +export function getImportScanningCacheStats(): {directImports: number; fileExists: number; isDir: number} { + return { + directImports: directImportsCache.size, + fileExists: fileExistsCache.size, + isDir: isDirCache.size, + } +} + /** * Extracts import paths from a JavaScript content. * @@ -139,7 +191,7 @@ function extractRustImports(content: string, filePath: string): string[] { const pathValue = match[1] if (pathValue) { const resolvedPath = joinPath(dirname(filePath), pathValue) - if (fileExistsSync(resolvedPath)) { + if (cachedFileExists(resolvedPath)) { imports.push(resolvedPath) } } @@ -149,11 +201,10 @@ function extractRustImports(content: string, filePath: string): string[] { } function resolveJSImport(importPath: string, fromFile: string): string | null { - const basePath = fileExistsSync(fromFile) && isDirectorySync(fromFile) ? fromFile : dirname(fromFile) + const basePath = cachedFileExists(fromFile) && cachedIsDir(fromFile) ? fromFile : dirname(fromFile) const resolvedPath = joinPath(basePath, importPath) - // If the import path resolves to a directory, look for index files - if (fileExistsSync(resolvedPath) && isDirectorySync(resolvedPath)) { + if (cachedFileExists(resolvedPath) && cachedIsDir(resolvedPath)) { const indexPaths = [ joinPath(resolvedPath, 'index.js'), joinPath(resolvedPath, 'index.ts'), @@ -162,15 +213,13 @@ function resolveJSImport(importPath: string, fromFile: string): string | null { ] for (const indexPath of indexPaths) { - if (fileExistsSync(indexPath) && !isDirectorySync(indexPath)) { + if (cachedFileExists(indexPath) && !cachedIsDir(indexPath)) { return indexPath } } - // If no index file found, don't return the directory return null } - // Check for file with extensions const possiblePaths = [ resolvedPath, `${resolvedPath}.js`, @@ -180,7 +229,7 @@ function resolveJSImport(importPath: string, fromFile: string): string | null { ] for (const path of possiblePaths) { - if (fileExistsSync(path) && !isDirectorySync(path)) { + if (cachedFileExists(path) && !cachedIsDir(path)) { return path } } @@ -193,7 +242,7 @@ function resolveRustModule(modName: string, fromFile: string): string | null { const possiblePaths = [joinPath(basePath, `${modName}.rs`), joinPath(basePath, modName, 'mod.rs')] for (const path of possiblePaths) { - if (fileExistsSync(path)) { + if (cachedFileExists(path)) { return path } }