From 9ccefb34693911d284904fecea66a6ff70f283fa Mon Sep 17 00:00:00 2001 From: Lars Kappert Date: Sun, 9 Jun 2024 12:38:24 +0200 Subject: [PATCH] Improve re-export handling --- packages/knip/src/index.ts | 15 +++++-- packages/knip/src/types/dependency-graph.ts | 1 + .../src/typescript/getImportsAndExports.ts | 39 +++++++++++-------- .../knip/src/util/is-identifier-referenced.ts | 38 +++++++++--------- packages/knip/test/fix-workspaces.test.ts | 2 +- packages/knip/test/fix.test.ts | 12 +++++- .../test/re-exports-with-decorator.test.ts | 2 +- packages/knip/test/re-exports.test.ts | 2 +- packages/knip/test/workspaces-dts.test.ts | 2 +- 9 files changed, 68 insertions(+), 45 deletions(-) diff --git a/packages/knip/src/index.ts b/packages/knip/src/index.ts index 52cf33004..b76e03f0a 100644 --- a/packages/knip/src/index.ts +++ b/packages/knip/src/index.ts @@ -408,19 +408,26 @@ export const main = async (unresolvedConfiguration: CommandLineOptions) => { const importsForExport = file.imported; for (const [identifier, exportedItem] of exportItems.entries()) { + if (exportedItem.isReExport) continue; + // Skip tagged exports if (shouldIgnore(exportedItem.jsDocTags)) continue; if (importsForExport) { - const { isReferenced, hasReExportingEntryFile, traceNode } = isIdentifierReferenced( + const { isReferenced, reExportingEntryFile, traceNode } = isIdentifierReferenced( filePath, identifier, isIncludeEntryExports ); - if (!isIncludeEntryExports && hasReExportingEntryFile) { - createAndPrintTrace(filePath, { identifier, isEntry, hasRef: isReferenced }); - continue; + if (reExportingEntryFile) { + if (!isIncludeEntryExports) { + createAndPrintTrace(filePath, { identifier, isEntry, hasRef: isReferenced }); + continue; + } + // Skip exports if re-exported from entry file and tagged + const reExportedItem = graph.get(reExportingEntryFile)?.exports.exported.get(identifier); + if (reExportedItem && shouldIgnore(reExportedItem.jsDocTags)) continue; } if (traceNode) printTrace(traceNode, filePath, identifier); diff --git a/packages/knip/src/types/dependency-graph.ts b/packages/knip/src/types/dependency-graph.ts index a8d776705..b3e3c8b8c 100644 --- a/packages/knip/src/types/dependency-graph.ts +++ b/packages/knip/src/types/dependency-graph.ts @@ -39,6 +39,7 @@ export interface Export { refs: number; fixes: Fixes; symbol?: ts.Symbol; + isReExport: boolean; } export type ExportMember = { diff --git a/packages/knip/src/typescript/getImportsAndExports.ts b/packages/knip/src/typescript/getImportsAndExports.ts index 3b000cf79..5821d4ac6 100644 --- a/packages/knip/src/typescript/getImportsAndExports.ts +++ b/packages/knip/src/typescript/getImportsAndExports.ts @@ -103,34 +103,35 @@ const getImportsAndExports = ( if (!file) internalImports.set(filePath, imports); + const nsOrAlias = symbol ? String(symbol.escapedName) : options.alias; + if (isReExport) { if (isStar && namespace) { // Pattern: export * as NS from 'specifier'; addValue(imports.reExportedNs, namespace, sourceFile.fileName); - } else if (namespace) { + } else if (nsOrAlias) { // Pattern: export { id as alias } from 'specifier'; - addNsValue(imports.reExportedAs, identifier, namespace, sourceFile.fileName); + addNsValue(imports.reExportedAs, identifier, nsOrAlias, sourceFile.fileName); } else { // Patterns: // export { id } from 'specifier'; // export * from 'specifier'; addValue(imports.reExported, identifier, sourceFile.fileName); } - } - - const nsOrAlias = symbol ? String(symbol.escapedName) : options.alias; - if (nsOrAlias && nsOrAlias !== identifier) { - if (isStar) { - addValue(imports.importedNs, nsOrAlias, sourceFile.fileName); - } else { - addNsValue(imports.importedAs, identifier, nsOrAlias, sourceFile.fileName); + } else { + if (nsOrAlias && nsOrAlias !== identifier) { + if (isStar) { + addValue(imports.importedNs, nsOrAlias, sourceFile.fileName); + } else { + addNsValue(imports.importedAs, identifier, nsOrAlias, sourceFile.fileName); + } + } else if (identifier !== ANONYMOUS && identifier !== IMPORT_STAR) { + addValue(imports.imported, identifier, sourceFile.fileName); } - } else if (identifier !== ANONYMOUS && identifier !== IMPORT_STAR) { - addValue(imports.imported, identifier, sourceFile.fileName); - } - if (symbol && DEFAULT_EXTENSIONS.includes(extname(sourceFile.fileName))) { - importedInternalSymbols.set(symbol, filePath); + if (symbol && DEFAULT_EXTENSIONS.includes(extname(sourceFile.fileName))) { + importedInternalSymbols.set(symbol, filePath); + } } }; @@ -228,12 +229,17 @@ const getImportsAndExports = ( const exportMembers = members.map(member => createMember(node, member, member.pos)); + const isReExport = Boolean( + node.parent?.parent && ts.isExportDeclaration(node.parent.parent) && node.parent.parent.moduleSpecifier + ); + const item = exports.get(identifier); if (item) { + // Code path for fn overloads, simple merge const members = [...(item.members ?? []), ...exportMembers]; const tags = new Set([...(item.jsDocTags ?? []), ...jsDocTags]); const fixes = fix ? [...(item.fixes ?? []), fix] : item.fixes; - exports.set(identifier, { ...item, members, jsDocTags: tags, fixes }); + exports.set(identifier, { ...item, members, jsDocTags: tags, fixes, isReExport }); } else { const { line, character } = node.getSourceFile().getLineAndCharacterOfPosition(pos); exports.set(identifier, { @@ -248,6 +254,7 @@ const getImportsAndExports = ( col: character + 1, fixes: fix ? [fix] : [], refs: 0, + isReExport, }); } diff --git a/packages/knip/src/util/is-identifier-referenced.ts b/packages/knip/src/util/is-identifier-referenced.ts index 29459eff0..23005223e 100644 --- a/packages/knip/src/util/is-identifier-referenced.ts +++ b/packages/knip/src/util/is-identifier-referenced.ts @@ -4,7 +4,7 @@ import { type TraceNode, addNodes, createNode, isTrace } from './trace.js'; type Result = { isReferenced: boolean; - hasReExportingEntryFile: boolean; + reExportingEntryFile: undefined | string; traceNode: TraceNode; }; @@ -17,11 +17,11 @@ export const getIsIdentifierReferencedHandler = (graph: DependencyGraph, entryPa seen = new Set() ): Result => { let isReferenced = false; - let hasReExportingEntryFile = entryPaths.has(filePath); + let reExportingEntryFile = entryPaths.has(filePath) ? filePath : undefined; - if (hasReExportingEntryFile) traceNode.isEntry = true; + if (reExportingEntryFile) traceNode.isEntry = true; - if (!isIncludeEntryExports && hasReExportingEntryFile) return { isReferenced, hasReExportingEntryFile, traceNode }; + if (!isIncludeEntryExports && reExportingEntryFile) return { isReferenced, reExportingEntryFile, traceNode }; seen.add(filePath); @@ -30,14 +30,14 @@ export const getIsIdentifierReferencedHandler = (graph: DependencyGraph, entryPa const file = graph.get(filePath)?.imported; - if (!file) return { isReferenced, hasReExportingEntryFile, traceNode }; + if (!file) return { isReferenced, reExportingEntryFile, traceNode }; if ( ((identifier !== id && file.refs.has(id)) || identifier === id) && (file.imported.has(identifier) || file.importedAs.has(identifier)) ) { isReferenced = true; - if (!isTrace) return { isReferenced, hasReExportingEntryFile, traceNode }; + if (!isTrace) return { isReferenced, reExportingEntryFile, traceNode }; addNodes(traceNode, id, graph, file.imported.get(identifier)); } @@ -47,7 +47,7 @@ export const getIsIdentifierReferencedHandler = (graph: DependencyGraph, entryPa const aliasedRef = [alias, ...rest].join('.'); if (file.refs.has(aliasedRef)) { isReferenced = true; - if (!isTrace) return { isReferenced, hasReExportingEntryFile, traceNode }; + if (!isTrace) return { isReferenced, reExportingEntryFile, traceNode }; addNodes(traceNode, aliasedRef, graph, aliases.get(alias)); } } @@ -57,7 +57,7 @@ export const getIsIdentifierReferencedHandler = (graph: DependencyGraph, entryPa for (const [namespace, byFilePaths] of file.importedNs) { if (file.refs.has(`${namespace}.${id}`)) { isReferenced = true; - if (!isTrace) return { isReferenced, hasReExportingEntryFile, traceNode }; + if (!isTrace) return { isReferenced, reExportingEntryFile, traceNode }; addNodes(traceNode, `${namespace}.${id}`, graph, byFilePaths); } @@ -70,10 +70,10 @@ export const getIsIdentifierReferencedHandler = (graph: DependencyGraph, entryPa const child = createNode(byFilePath); traceNode.children.add(child); const result = isIdentifierReferenced(byFilePath, `${alias}.${id}`, isIncludeEntryExports, child, seen); - if (result.hasReExportingEntryFile) hasReExportingEntryFile = true; + if (result.reExportingEntryFile) reExportingEntryFile = result.reExportingEntryFile; if (result.isReferenced) { isReferenced = true; - if (!isTrace) return { isReferenced, hasReExportingEntryFile, traceNode }; + if (!isTrace) return { isReferenced, reExportingEntryFile, traceNode }; } } } @@ -88,10 +88,10 @@ export const getIsIdentifierReferencedHandler = (graph: DependencyGraph, entryPa const child = createNode(byFilePath); traceNode.children.add(child); const result = isIdentifierReferenced(byFilePath, `${namespace}.${id}`, isIncludeEntryExports, child, seen); - if (result.hasReExportingEntryFile) hasReExportingEntryFile = true; + if (result.reExportingEntryFile) reExportingEntryFile = result.reExportingEntryFile; if (result.isReferenced) { isReferenced = true; - if (!isTrace) return { isReferenced, hasReExportingEntryFile, traceNode }; + if (!isTrace) return { isReferenced, reExportingEntryFile, traceNode }; } } } @@ -106,10 +106,10 @@ export const getIsIdentifierReferencedHandler = (graph: DependencyGraph, entryPa const child = createNode(byFilePath); traceNode.children.add(child); const result = isIdentifierReferenced(byFilePath, id, isIncludeEntryExports, child, seen); - if (result.hasReExportingEntryFile) hasReExportingEntryFile = true; + if (result.reExportingEntryFile) reExportingEntryFile = result.reExportingEntryFile; if (result.isReferenced) { isReferenced = true; - if (!isTrace) return { isReferenced, hasReExportingEntryFile, traceNode }; + if (!isTrace) return { isReferenced, reExportingEntryFile, traceNode }; } } } @@ -125,10 +125,10 @@ export const getIsIdentifierReferencedHandler = (graph: DependencyGraph, entryPa traceNode.children.add(child); const ref = [alias, ...rest].join('.'); const result = isIdentifierReferenced(byFilePath, ref, isIncludeEntryExports, child, seen); - if (result.hasReExportingEntryFile) hasReExportingEntryFile = true; + if (result.reExportingEntryFile) reExportingEntryFile = result.reExportingEntryFile; if (result.isReferenced) { isReferenced = true; - if (!isTrace) return { isReferenced, hasReExportingEntryFile, traceNode }; + if (!isTrace) return { isReferenced, reExportingEntryFile, traceNode }; } } } @@ -141,16 +141,16 @@ export const getIsIdentifierReferencedHandler = (graph: DependencyGraph, entryPa const child = createNode(byFilePath); traceNode.children.add(child); const result = isIdentifierReferenced(byFilePath, `${namespace}.${id}`, isIncludeEntryExports, child, seen); - if (result.hasReExportingEntryFile) hasReExportingEntryFile = true; + if (result.reExportingEntryFile) reExportingEntryFile = result.reExportingEntryFile; if (result.isReferenced) { isReferenced = true; - if (!isTrace) return { isReferenced, hasReExportingEntryFile, traceNode }; + if (!isTrace) return { isReferenced, reExportingEntryFile, traceNode }; } } } } - return { isReferenced, hasReExportingEntryFile, traceNode }; + return { isReferenced, reExportingEntryFile, traceNode }; }; return isIdentifierReferenced; diff --git a/packages/knip/test/fix-workspaces.test.ts b/packages/knip/test/fix-workspaces.test.ts index 4463cba1b..daa1ba65e 100644 --- a/packages/knip/test/fix-workspaces.test.ts +++ b/packages/knip/test/fix-workspaces.test.ts @@ -10,7 +10,7 @@ const cwd = resolve('fixtures/fix-workspaces'); const readContents = async (fileName: string) => await readFile(join(cwd, fileName), 'utf8'); -test('Remove exports and dependencies', async () => { +test('Remove exports and dependencies (workspaces)', async () => { const tests = [ [ 'exports.ts', diff --git a/packages/knip/test/fix.test.ts b/packages/knip/test/fix.test.ts index f343ff452..75ffd8c73 100644 --- a/packages/knip/test/fix.test.ts +++ b/packages/knip/test/fix.test.ts @@ -54,7 +54,11 @@ module.exports = { identifier, }; [ 'reexports.js', await readContents('reexports.js'), - `export { One } from './reexported'; + `export { RangeSlider } from './reexported'; +export { Rating } from './reexported'; +export { One } from './reexported'; +export { Col, Col as KCol } from './reexported'; +export { Row as KRow, Row } from './reexported'; `.replace(/\n/g, EOL), ], [ @@ -89,14 +93,18 @@ export const One = 1; tags: [[], ['knipignore']], }); - assert(issues.exports['access.js']['ACCESS']); assert(issues.exports['access.js']['UNUSED']); + assert(issues.exports['access.js']['ACCESS']); assert(issues.exports['exports.js']['identifier2']); assert(issues.exports['mod.ts']['a']); assert(issues.exports['mod.ts']['b']); + assert(issues.exports['mod.ts']['c']); + assert(issues.exports['mod.ts']['d']); assert(issues.exports['mod.ts']['default']); assert(issues.exports['mod.ts']['x']); assert(issues.exports['mod.ts']['y']); + assert(issues.exports['reexported.js']['Three']); + assert(issues.exports['reexported.js']['Two']); // check ignore assert(issues.exports['ignored.ts'] === undefined); diff --git a/packages/knip/test/re-exports-with-decorator.test.ts b/packages/knip/test/re-exports-with-decorator.test.ts index 618601059..6dd51b3ae 100644 --- a/packages/knip/test/re-exports-with-decorator.test.ts +++ b/packages/knip/test/re-exports-with-decorator.test.ts @@ -7,7 +7,7 @@ import baseCounters from './helpers/baseCounters.js'; const cwd = resolve('fixtures/re-exports-with-decorator'); -test('Ignore re-exports from entry files', async () => { +test('Ignore re-exports from entry file (w/ decorator)', async () => { const { counters } = await main({ ...baseArguments, cwd, diff --git a/packages/knip/test/re-exports.test.ts b/packages/knip/test/re-exports.test.ts index d65cb9fa3..36af142f0 100644 --- a/packages/knip/test/re-exports.test.ts +++ b/packages/knip/test/re-exports.test.ts @@ -27,7 +27,7 @@ test('Ignore re-exports from entry files (include entry + ignore @public)', asyn isIncludeEntryExports: true, }); - assert(issues.exports['1-entry.ts']['somethingNotToIgnore']); + assert(issues.exports['4-my-module.ts']['somethingNotToIgnore']); assert.deepEqual(counters, { ...baseCounters, diff --git a/packages/knip/test/workspaces-dts.test.ts b/packages/knip/test/workspaces-dts.test.ts index fe9511d75..aa7bd5b19 100644 --- a/packages/knip/test/workspaces-dts.test.ts +++ b/packages/knip/test/workspaces-dts.test.ts @@ -13,7 +13,7 @@ test('Find unused un-built exports across workspaces', async () => { cwd, }); - assert(issues.exports['packages/shared/src/index.js']['unusedFunction']); + assert(issues.exports['packages/shared/src/unused-function.js']['unusedFunction']); assert.deepEqual(counters, { ...baseCounters,