diff --git a/.changeset/skip-external-type-generation-imports.md b/.changeset/skip-external-type-generation-imports.md new file mode 100644 index 00000000000..b7360155bb5 --- /dev/null +++ b/.changeset/skip-external-type-generation-imports.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli': patch +--- + +Improve extension type generation by avoiding unnecessary scans outside extension source files. diff --git a/packages/app/src/cli/models/extensions/specifications/type-generation.test.ts b/packages/app/src/cli/models/extensions/specifications/type-generation.test.ts index b3029f3599e..4593c539bf8 100644 --- a/packages/app/src/cli/models/extensions/specifications/type-generation.test.ts +++ b/packages/app/src/cli/models/extensions/specifications/type-generation.test.ts @@ -1,10 +1,15 @@ import { createIntentsTypeDefinition, createToolsTypeDefinition, + findExplicitTsConfigFiles, + findAllImportedFiles, getGeneratedTypesHelperImportPath, } from './type-generation.js' +import * as fs from '@shopify/cli-kit/node/fs' import {AbortError} from '@shopify/cli-kit/node/error' -import {describe, expect, test} from 'vitest' +import {inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs' +import {joinPath, normalizePath} from '@shopify/cli-kit/node/path' +import {describe, expect, test, vi} from 'vitest' const adminGeneratedTypesHelperImportPath = '@shopify/ui-extensions/admin' @@ -21,6 +26,283 @@ describe('getGeneratedTypesHelperImportPath', () => { }) }) +describe('findAllImportedFiles', () => { + test('ignores commented-out imports', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const entryPath = joinPath(tmpDir, 'index.ts') + const commentedPath = joinPath(tmpDir, 'commented.ts') + + await writeFile( + entryPath, + ` + // import './commented.ts' + /* + import './commented.ts' + */ + `, + ) + await writeFile(commentedPath, `export const commented = true`) + + const importedFiles = (await findAllImportedFiles(entryPath)).map((file) => normalizePath(file)) + + expect(importedFiles).not.toContain(normalizePath(commentedPath)) + }) + }) + + test('does not follow type-only imports and exports', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const entryPath = joinPath(tmpDir, 'index.ts') + const valuePath = joinPath(tmpDir, 'value.ts') + const valueNestedPath = joinPath(tmpDir, 'value-nested.ts') + const mixedValuePath = joinPath(tmpDir, 'mixed-value.ts') + const mixedExportPath = joinPath(tmpDir, 'mixed-export.ts') + const typePath = joinPath(tmpDir, 'types.ts') + const typeNestedPath = joinPath(tmpDir, 'type-nested.ts') + const exportedTypePath = joinPath(tmpDir, 'exported-types.ts') + const exportedTypeNestedPath = joinPath(tmpDir, 'exported-type-nested.ts') + const emptyImportPath = joinPath(tmpDir, 'empty-import.ts') + const emptyImportNestedPath = joinPath(tmpDir, 'empty-import-nested.ts') + const emptyExportPath = joinPath(tmpDir, 'empty-export.ts') + const emptyExportNestedPath = joinPath(tmpDir, 'empty-export-nested.ts') + + await writeFile( + entryPath, + ` + import './value.ts' + import MixedValue, { type MixedType } from './mixed-value.ts' + import {} from './empty-import.ts' + import type { TypeOnly } from './types.ts' + export { mixedValue, type MixedExportType } from './mixed-export.ts' + export {} from './empty-export.ts' + export type { ExportedTypeOnly } from './exported-types.ts' + `, + ) + await writeFile(valuePath, `import './value-nested.ts'`) + await writeFile(valueNestedPath, `export const valueNested = true`) + await writeFile(mixedValuePath, `export default true; export type MixedType = string`) + await writeFile(mixedExportPath, `export const mixedValue = true; export type MixedExportType = string`) + await writeFile(typePath, `import './type-nested.ts'; export type TypeOnly = string`) + await writeFile(typeNestedPath, `export const typeNested = true`) + await writeFile(exportedTypePath, `import './exported-type-nested.ts'; export type ExportedTypeOnly = string`) + await writeFile(exportedTypeNestedPath, `export const exportedTypeNested = true`) + await writeFile(emptyImportPath, `import './empty-import-nested.ts'`) + await writeFile(emptyImportNestedPath, `export const emptyImportNested = true`) + await writeFile(emptyExportPath, `import './empty-export-nested.ts'`) + await writeFile(emptyExportNestedPath, `export const emptyExportNested = true`) + + const importedFiles = (await findAllImportedFiles(entryPath)).map((file) => normalizePath(file)) + + expect(importedFiles).toContain(normalizePath(valuePath)) + expect(importedFiles).toContain(normalizePath(valueNestedPath)) + expect(importedFiles).toContain(normalizePath(mixedValuePath)) + expect(importedFiles).toContain(normalizePath(mixedExportPath)) + expect(importedFiles).toContain(normalizePath(emptyImportPath)) + expect(importedFiles).toContain(normalizePath(emptyImportNestedPath)) + expect(importedFiles).toContain(normalizePath(emptyExportPath)) + expect(importedFiles).toContain(normalizePath(emptyExportNestedPath)) + expect(importedFiles).not.toContain(normalizePath(typePath)) + expect(importedFiles).not.toContain(normalizePath(typeNestedPath)) + expect(importedFiles).not.toContain(normalizePath(exportedTypePath)) + expect(importedFiles).not.toContain(normalizePath(exportedTypeNestedPath)) + }) + }) + + test('stops recursive import scanning at the boundary directory', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const extensionDir = joinPath(tmpDir, 'extensions', 'extension') + const srcDir = joinPath(extensionDir, 'src') + const sharedDir = joinPath(tmpDir, 'shared') + + await mkdir(extensionDir) + await mkdir(srcDir) + await mkdir(sharedDir) + + const entryPath = joinPath(srcDir, 'index.ts') + const localPath = joinPath(srcDir, 'local.ts') + const nestedPath = joinPath(srcDir, 'nested.ts') + const externalPath = joinPath(sharedDir, 'utils.ts') + const externalNestedPath = joinPath(sharedDir, 'secret.ts') + + await writeFile( + entryPath, + ` + import './local.ts' + import '../../../shared/utils.ts' + `, + ) + await writeFile(localPath, `import './nested.ts'`) + await writeFile(nestedPath, `export const nested = true`) + await writeFile(externalPath, `import './secret.ts'`) + await writeFile(externalNestedPath, `export const secret = true`) + + const importedFiles = (await findAllImportedFiles(entryPath, {boundaryDirectory: extensionDir})).map((file) => + normalizePath(file), + ) + + expect(importedFiles).toContain(normalizePath(localPath)) + expect(importedFiles).toContain(normalizePath(nestedPath)) + expect(importedFiles).not.toContain(normalizePath(externalPath)) + expect(importedFiles).not.toContain(normalizePath(externalNestedPath)) + }) + }) + + test('only follows files from an explicit tsconfig include list when provided', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const srcDir = joinPath(tmpDir, 'src') + const excludedDir = joinPath(tmpDir, 'excluded') + + await mkdir(srcDir) + await mkdir(excludedDir) + + const entryPath = joinPath(srcDir, 'index.ts') + const includedPath = joinPath(srcDir, 'included.ts') + const includedNestedPath = joinPath(srcDir, 'included-nested.ts') + const excludedPath = joinPath(excludedDir, 'excluded.ts') + const excludedNestedPath = joinPath(excludedDir, 'excluded-nested.ts') + + await writeFile( + joinPath(tmpDir, 'tsconfig.json'), + JSON.stringify({ + include: ['src/**/*'], + }), + ) + await writeFile( + entryPath, + ` + import './included.ts' + import '../excluded/excluded.ts' + `, + ) + await writeFile(includedPath, `import './included-nested.ts'`) + await writeFile(includedNestedPath, `export const includedNested = true`) + await writeFile(excludedPath, `import './excluded-nested.ts'`) + await writeFile(excludedNestedPath, `export const excludedNested = true`) + + const allowedFiles = await findExplicitTsConfigFiles(entryPath, tmpDir) + const importedFiles = ( + await findAllImportedFiles(entryPath, { + boundaryDirectory: tmpDir, + allowedFiles, + alwaysAllowedFiles: new Set([entryPath]), + }) + ).map((file) => normalizePath(file)) + + expect(importedFiles).toContain(normalizePath(includedPath)) + expect(importedFiles).toContain(normalizePath(includedNestedPath)) + expect(importedFiles).not.toContain(normalizePath(excludedPath)) + expect(importedFiles).not.toContain(normalizePath(excludedNestedPath)) + }) + }) + + test('uses explicit tsconfig include inherited from an extended config', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const extensionDir = joinPath(tmpDir, 'extensions', 'extension') + const srcDir = joinPath(extensionDir, 'src') + const excludedDir = joinPath(extensionDir, 'excluded') + + await mkdir(extensionDir) + await mkdir(srcDir) + await mkdir(excludedDir) + + const entryPath = joinPath(srcDir, 'index.ts') + const includedPath = joinPath(srcDir, 'included.ts') + const excludedPath = joinPath(excludedDir, 'excluded.ts') + + await writeFile( + joinPath(tmpDir, 'tsconfig.base.json'), + JSON.stringify({ + include: ['extensions/extension/src/**/*'], + }), + ) + await writeFile( + joinPath(extensionDir, 'tsconfig.json'), + JSON.stringify({ + extends: '../../tsconfig.base.json', + }), + ) + await writeFile( + entryPath, + ` + import './included.ts' + import '../excluded/excluded.ts' + `, + ) + await writeFile(includedPath, `export const included = true`) + await writeFile(excludedPath, `export const excluded = true`) + + const allowedFiles = await findExplicitTsConfigFiles(entryPath, extensionDir) + const importedFiles = ( + await findAllImportedFiles(entryPath, { + boundaryDirectory: extensionDir, + allowedFiles, + alwaysAllowedFiles: new Set([entryPath]), + }) + ).map((file) => normalizePath(file)) + + expect(allowedFiles).toContain(normalizePath(entryPath)) + expect(importedFiles).toContain(normalizePath(includedPath)) + expect(importedFiles).not.toContain(normalizePath(excludedPath)) + }) + }) + + test('does not follow declaration files', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const entryPath = joinPath(tmpDir, 'index.ts') + const declarationPath = joinPath(tmpDir, 'types.d.ts') + const nestedPath = joinPath(tmpDir, 'nested.ts') + + await writeFile(entryPath, `import './types.d.ts'`) + await writeFile(declarationPath, `import './nested.ts'`) + await writeFile(nestedPath, `export const nested = true`) + + const importedFiles = (await findAllImportedFiles(entryPath, {boundaryDirectory: tmpDir})).map((file) => + normalizePath(file), + ) + + expect(importedFiles).not.toContain(normalizePath(declarationPath)) + expect(importedFiles).not.toContain(normalizePath(nestedPath)) + }) + }) + + test('does not use a tsconfig allowlist when files and include are implicit', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const entryPath = joinPath(tmpDir, 'index.ts') + + await writeFile(joinPath(tmpDir, 'tsconfig.json'), '{}') + await writeFile(entryPath, `export const entry = true`) + + await expect(findExplicitTsConfigFiles(entryPath, tmpDir)).resolves.toBeUndefined() + }) + }) + + test('uses cached imports when the same file is scanned more than once', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const entryPath = joinPath(tmpDir, 'index.ts') + const localPath = joinPath(tmpDir, 'local.ts') + const nestedPath = joinPath(tmpDir, 'nested.ts') + + await writeFile(entryPath, `import './local.ts'`) + await writeFile(localPath, `import './nested.ts'`) + await writeFile(nestedPath, `export const nested = true`) + + const readFileSync = vi.spyOn(fs, 'readFileSync') + const importCache = new Map() + + await findAllImportedFiles(entryPath, {boundaryDirectory: tmpDir, importCache}) + await findAllImportedFiles(entryPath, {boundaryDirectory: tmpDir, importCache}) + + const readCountsByPath = new Map() + for (const [path] of readFileSync.mock.calls) { + readCountsByPath.set(path, (readCountsByPath.get(path) ?? 0) + 1) + } + + expect(readCountsByPath.get(entryPath)).toBe(1) + expect(readCountsByPath.get(localPath)).toBe(1) + expect(readCountsByPath.get(nestedPath)).toBe(1) + }) + }) +}) + describe('createIntentsTypeDefinition', () => { test('returns empty string when intents array is empty', async () => { // When diff --git a/packages/app/src/cli/models/extensions/specifications/type-generation.ts b/packages/app/src/cli/models/extensions/specifications/type-generation.ts index 32da0e44a6d..59976b52f72 100644 --- a/packages/app/src/cli/models/extensions/specifications/type-generation.ts +++ b/packages/app/src/cli/models/extensions/specifications/type-generation.ts @@ -1,5 +1,5 @@ import {fileExists, findPathUp, readFileSync} from '@shopify/cli-kit/node/fs' -import {dirname, joinPath, relativizePath, resolvePath} from '@shopify/cli-kit/node/path' +import {dirname, isSubpath, joinPath, relativizePath, resolvePath} from '@shopify/cli-kit/node/path' import {AbortError} from '@shopify/cli-kit/node/error' import {compile} from 'json-schema-to-typescript' import {pascalize} from '@shopify/cli-kit/common/string' @@ -46,23 +46,44 @@ export function parseApiVersion(apiVersion: string): {year: number; month: numbe return {year: parseInt(year, 10), month: parseInt(month, 10)} } -async function loadTsConfig( - startPath: string, -): Promise<{compilerOptions: ts.CompilerOptions; configPath: string | undefined}> { +interface LoadedTsConfig { + compilerOptions: ts.CompilerOptions + configPath: string | undefined + fileNames?: string[] + hasExplicitFiles: boolean +} + +export type TsConfigCache = Map + +async function loadTsConfig(startPath: string, cache?: TsConfigCache): Promise { const ts = await loadTypeScript() const configPath = ts.findConfigFile(startPath, ts.sys.fileExists.bind(ts.sys), 'tsconfig.json') if (!configPath) { - return {compilerOptions: {}, configPath: undefined} + return {compilerOptions: {}, configPath: undefined, hasExplicitFiles: false} + } + + const resolvedConfigPath = resolvePath(configPath) + const cachedConfig = cache?.get(resolvedConfigPath) + if (cachedConfig) { + return cachedConfig } const configFile = ts.readConfigFile(configPath, ts.sys.readFile.bind(ts.sys)) if (configFile.error) { - return {compilerOptions: {}, configPath} + return {compilerOptions: {}, configPath: resolvedConfigPath, hasExplicitFiles: false} } const parsedConfig = ts.parseJsonConfigFileContent(configFile.config, ts.sys, dirname(configPath)) + const hasExplicitFiles = Boolean(parsedConfig.raw?.files ?? parsedConfig.raw?.include) + const loadedConfig = { + compilerOptions: parsedConfig.options, + configPath: resolvedConfigPath, + fileNames: parsedConfig.fileNames, + hasExplicitFiles, + } - return {compilerOptions: parsedConfig.options, configPath} + cache?.set(resolvedConfigPath, loadedConfig) + return loadedConfig } async function fallbackResolve(importPath: string, baseDir: string): Promise { @@ -95,14 +116,57 @@ async function fallbackResolve(importPath: string, baseDir: string): Promise { +interface FindAllImportedFilesOptions { + boundaryDirectory?: string + allowedFiles?: Set + alwaysAllowedFiles?: Set + importCache?: Map + tsConfigCache?: TsConfigCache +} + +interface ParseAndResolveImportsOptions { + importCache?: Map + tsConfigCache?: TsConfigCache +} + +function isWithinBoundary(filePath: string, boundaryDirectory?: string): boolean { + if (!boundaryDirectory) return true + return isSubpath(resolvePath(boundaryDirectory), resolvePath(filePath)) +} + +function isAllowedFile(filePath: string, options: FindAllImportedFilesOptions): boolean { + if (!options.allowedFiles) return true + + const resolvedPath = resolvePath(filePath) + return options.allowedFiles.has(resolvedPath) || Boolean(options.alwaysAllowedFiles?.has(resolvedPath)) +} + +function isScannableFile(filePath: string, options: FindAllImportedFilesOptions): boolean { + return ( + !filePath.includes('node_modules') && + !filePath.endsWith('.d.ts') && + isWithinBoundary(filePath, options.boundaryDirectory) && + isAllowedFile(filePath, options) + ) +} + +async function parseAndResolveImports( + filePath: string, + options: ParseAndResolveImportsOptions = {}, +): Promise { try { + const resolvedFilePath = resolvePath(filePath) + const cachedImports = options.importCache?.get(resolvedFilePath) + if (cachedImports) { + return cachedImports + } + const ts = await loadTypeScript() const content = readFileSync(filePath).toString() const resolvedPaths: string[] = [] // Load TypeScript configuration once - const {compilerOptions} = await loadTsConfig(filePath) + const {compilerOptions} = await loadTsConfig(filePath, options.tsConfigCache) // Determine script kind based on file extension let scriptKind = ts.ScriptKind.JSX @@ -119,6 +183,20 @@ async function parseAndResolveImports(filePath: string): Promise { const visit = (node: ts.Node): void => { if (ts.isImportDeclaration(node) && node.moduleSpecifier && ts.isStringLiteral(node.moduleSpecifier)) { + if (node.importClause?.isTypeOnly) { + return + } + + if ( + !node.importClause?.name && + node.importClause?.namedBindings && + ts.isNamedImports(node.importClause.namedBindings) && + node.importClause.namedBindings.elements.length > 0 && + node.importClause.namedBindings.elements.every((element) => element.isTypeOnly) + ) { + return + } + importPaths.push(node.moduleSpecifier.text) } else if (ts.isCallExpression(node) && node.expression.kind === ts.SyntaxKind.ImportKeyword) { const firstArg = node.arguments[0] @@ -126,6 +204,19 @@ async function parseAndResolveImports(filePath: string): Promise { importPaths.push(firstArg.text) } } else if (ts.isExportDeclaration(node) && node.moduleSpecifier && ts.isStringLiteral(node.moduleSpecifier)) { + if (node.isTypeOnly) { + return + } + + if ( + node.exportClause && + ts.isNamedExports(node.exportClause) && + node.exportClause.elements.length > 0 && + node.exportClause.elements.every((element) => element.isTypeOnly) + ) { + return + } + importPaths.push(node.moduleSpecifier.text) } @@ -147,9 +238,7 @@ async function parseAndResolveImports(filePath: string): Promise { if (resolvedModule.resolvedModule?.resolvedFileName) { const resolvedPath = resolvedModule.resolvedModule.resolvedFileName - if (!resolvedPath.includes('node_modules')) { - resolvedPaths.push(resolvedPath) - } + resolvedPaths.push(resolvedPath) } else { // Fallback to manual resolution for edge cases // eslint-disable-next-line no-await-in-loop @@ -160,6 +249,7 @@ async function parseAndResolveImports(filePath: string): Promise { } } + options.importCache?.set(resolvedFilePath, resolvedPaths) return resolvedPaths } catch (error) { // Re-throw AbortError as-is, wrap other errors @@ -170,26 +260,49 @@ async function parseAndResolveImports(filePath: string): Promise { } } -export async function findAllImportedFiles(filePath: string, visited = new Set()): Promise { +export async function findAllImportedFiles( + filePath: string, + options: FindAllImportedFilesOptions = {}, + visited = new Set(), +): Promise { if (visited.has(filePath)) { return [] } visited.add(filePath) - const resolvedPaths = await parseAndResolveImports(filePath) + const resolvedPaths = (await parseAndResolveImports(filePath, options)).filter((resolvedPath) => + isScannableFile(resolvedPath, options), + ) const allFiles = [...resolvedPaths] // Recursively find imports from the resolved files for (const resolvedPath of resolvedPaths) { // eslint-disable-next-line no-await-in-loop - const nestedImports = await findAllImportedFiles(resolvedPath, visited) + const nestedImports = await findAllImportedFiles(resolvedPath, options, visited) allFiles.push(...nestedImports) } return uniq(allFiles) } +export async function findExplicitTsConfigFiles( + fromFile: string, + extensionDirectory: string, + options: {tsConfigCache?: TsConfigCache} = {}, +): Promise | undefined> { + const {configPath, fileNames, hasExplicitFiles} = await loadTsConfig(fromFile, options.tsConfigCache) + if (!configPath || !hasExplicitFiles) return + + if (!isWithinBoundary(configPath, extensionDirectory)) return + + return new Set( + (fileNames ?? []) + .filter((fileName) => isWithinBoundary(fileName, extensionDirectory)) + .map((fileName) => resolvePath(fileName)), + ) +} + interface CreateTypeDefinitionOptions { fullPath: string typeFilePath: string diff --git a/packages/app/src/cli/models/extensions/specifications/ui_extension.ts b/packages/app/src/cli/models/extensions/specifications/ui_extension.ts index 594ed9e27cc..0312bc80480 100644 --- a/packages/app/src/cli/models/extensions/specifications/ui_extension.ts +++ b/packages/app/src/cli/models/extensions/specifications/ui_extension.ts @@ -4,9 +4,11 @@ import { createTypeDefinition, createToolsTypeDefinition, findNearestTsConfigDir, + findExplicitTsConfigFiles, getGeneratedTypesHelperImportPath, IntentSchemaFileSchema, parseApiVersion, + type TsConfigCache, ToolsFileSchema, } from './type-generation.js' import {Asset, AssetIdentifier, BuildAsset, ExtensionFeature, createExtensionSpecification} from '../specification.js' @@ -18,7 +20,7 @@ import {formatContent} from '../../../utilities/file-formatter.js' import {uniq} from '@shopify/cli-kit/common/array' import {err, ok, Result} from '@shopify/cli-kit/node/result' import {fileExists, readFile} from '@shopify/cli-kit/node/fs' -import {joinPath} from '@shopify/cli-kit/node/path' +import {joinPath, resolvePath} from '@shopify/cli-kit/node/path' import {outputContent, outputToken, outputWarn} from '@shopify/cli-kit/node/output' import {zod} from '@shopify/cli-kit/node/schema' import {AbortError} from '@shopify/cli-kit/node/error' @@ -210,6 +212,8 @@ const uiExtensionSpec = createExtensionSpecification({ const fileToTargetsMap = new Map() const fileToToolsMap = new Map() const fileToIntentsMap = new Map>() + const importCache = new Map() + const tsConfigCache: TsConfigCache = new Map() // First pass: collect all entry point files and their targets for await (const extensionPoint of configuration.extension_points) { @@ -254,7 +258,14 @@ const uiExtensionSpec = createExtensionSpecification({ if (!exists) continue // Find all imported files recursively - const importedFiles = await findAllImportedFiles(fullPath) + const allowedFiles = await findExplicitTsConfigFiles(fullPath, extension.directory, {tsConfigCache}) + const importedFiles = await findAllImportedFiles(fullPath, { + boundaryDirectory: extension.directory, + allowedFiles, + alwaysAllowedFiles: new Set([resolvePath(fullPath)]), + importCache, + tsConfigCache, + }) // Associate imported files with this extension point's target for (const importedFile of importedFiles) { @@ -271,7 +282,16 @@ const uiExtensionSpec = createExtensionSpecification({ ) const shouldRenderExists = await fileExists(shouldRenderPath) if (shouldRenderExists) { - const shouldRenderImports = await findAllImportedFiles(shouldRenderPath) + const shouldRenderAllowedFiles = await findExplicitTsConfigFiles(shouldRenderPath, extension.directory, { + tsConfigCache, + }) + const shouldRenderImports = await findAllImportedFiles(shouldRenderPath, { + boundaryDirectory: extension.directory, + allowedFiles: shouldRenderAllowedFiles, + alwaysAllowedFiles: new Set([resolvePath(shouldRenderPath)]), + importCache, + tsConfigCache, + }) for (const importedFile of shouldRenderImports) { const currentTargets = fileToTargetsMap.get(importedFile) ?? [] currentTargets.push(getShouldRenderTarget(extensionPoint.target))