-
Notifications
You must be signed in to change notification settings - Fork 604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[api-extractor] Fix incorrect declaration references for symbols not exported from the package's entry point #3584
Changes from 1 commit
d61f74f
b68fec6
19f91a8
7f2b01a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,38 +10,26 @@ import { | |
Navigation, | ||
Meaning | ||
} from '@microsoft/tsdoc/lib-commonjs/beta/DeclarationReference'; | ||
import { PackageJsonLookup, INodePackageJson, InternalError } from '@rushstack/node-core-library'; | ||
import { INodePackageJson, InternalError } from '@rushstack/node-core-library'; | ||
import { TypeScriptHelpers } from '../analyzer/TypeScriptHelpers'; | ||
import { TypeScriptInternals } from '../analyzer/TypeScriptInternals'; | ||
import { Collector } from '../collector/Collector'; | ||
import { CollectorEntity } from '../collector/CollectorEntity'; | ||
|
||
export class DeclarationReferenceGenerator { | ||
public static readonly unknownReference: string = '?'; | ||
|
||
private _packageJsonLookup: PackageJsonLookup; | ||
private _workingPackageName: string; | ||
private _program: ts.Program; | ||
private _typeChecker: ts.TypeChecker; | ||
private _bundledPackageNames: ReadonlySet<string>; | ||
|
||
public constructor( | ||
packageJsonLookup: PackageJsonLookup, | ||
workingPackageName: string, | ||
program: ts.Program, | ||
typeChecker: ts.TypeChecker, | ||
bundledPackageNames: ReadonlySet<string> | ||
) { | ||
this._packageJsonLookup = packageJsonLookup; | ||
this._workingPackageName = workingPackageName; | ||
this._program = program; | ||
this._typeChecker = typeChecker; | ||
this._bundledPackageNames = bundledPackageNames; | ||
private _collector: Collector; | ||
|
||
public constructor(collector: Collector) { | ||
this._collector = collector; | ||
} | ||
|
||
/** | ||
* Gets the UID for a TypeScript Identifier that references a type. | ||
*/ | ||
public getDeclarationReferenceForIdentifier(node: ts.Identifier): DeclarationReference | undefined { | ||
const symbol: ts.Symbol | undefined = this._typeChecker.getSymbolAtLocation(node); | ||
const symbol: ts.Symbol | undefined = this._collector.typeChecker.getSymbolAtLocation(node); | ||
if (symbol !== undefined) { | ||
const isExpression: boolean = DeclarationReferenceGenerator._isInExpressionContext(node); | ||
return ( | ||
|
@@ -99,68 +87,48 @@ export class DeclarationReferenceGenerator { | |
); | ||
} | ||
|
||
private static _getNavigationToSymbol(symbol: ts.Symbol): Navigation | 'global' { | ||
private _getNavigationToSymbol(symbol: ts.Symbol): Navigation { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's an interesting canonical reference question: Consider the following example:
What should the canonical reference of
api-extractor-model produces the latter because the "synthetic" I feel like the correct canonical reference in this case is
and it would be odd for Regardless, I don't think we should solve this in this PR, just something to think about. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm.... A central principle of API Extractor is that it interprets declarations, imposing a semantics that is not technically present in the source code. This is what justifies rolling up declarations into a different file from where they were declared, discarding some .d.ts files entirely (because they aren't reachable from the entry point), and renaming API items to avoid naming conflicts. Your question is really about what semantics should be applied to an unexported declaration.
I guess the choice boils down to requirements: Is our priority that the canonical reference should help a tool to find the declaration in the .d.ts files? If so then it should probably be a physical location in the .d.ts rollup. If the .d.ts rollup feature is disabled, then maybe all canonical references should actually be physical locations such as (1). Alternatively, is our priority that the canonical reference should be a stable identifier, i.e. resistant to getting shuffled around by trivial changes? I think this is the design choice that API Extractor has pursued. In this case I guess we would go with (2) or (3). |
||
const declaration: ts.Declaration | undefined = TypeScriptHelpers.tryGetADeclaration(symbol); | ||
const sourceFile: ts.SourceFile | undefined = declaration?.getSourceFile(); | ||
const parent: ts.Symbol | undefined = TypeScriptInternals.getSymbolParent(symbol); | ||
// First, try to determine navigation to symbol via its parent. | ||
if (parent) { | ||
if ( | ||
parent.exports && | ||
DeclarationReferenceGenerator._isSameSymbol(parent.exports.get(symbol.escapedName), symbol) | ||
) { | ||
return Navigation.Exports; | ||
} | ||
if ( | ||
parent.members && | ||
DeclarationReferenceGenerator._isSameSymbol(parent.members.get(symbol.escapedName), symbol) | ||
) { | ||
return Navigation.Members; | ||
} | ||
if ( | ||
parent.globalExports && | ||
DeclarationReferenceGenerator._isSameSymbol(parent.globalExports.get(symbol.escapedName), symbol) | ||
) { | ||
return 'global'; | ||
} | ||
|
||
// If it's a global, then use an Exports navigation. | ||
if (sourceFile && !ts.isExternalModule(sourceFile)) { | ||
return Navigation.Exports; | ||
} | ||
|
||
// Next, try determining navigation to symbol by its node | ||
if (symbol.valueDeclaration) { | ||
const declaration: ts.Declaration = ts.isBindingElement(symbol.valueDeclaration) | ||
? ts.walkUpBindingElementsAndPatterns(symbol.valueDeclaration) | ||
: symbol.valueDeclaration; | ||
if (ts.isClassElement(declaration) && ts.isClassLike(declaration.parent)) { | ||
// class members are an "export" if they have the static modifier. | ||
return ts.getCombinedModifierFlags(declaration) & ts.ModifierFlags.Static | ||
? Navigation.Exports | ||
: Navigation.Members; | ||
} | ||
if (ts.isTypeElement(declaration) || ts.isObjectLiteralElement(declaration)) { | ||
// type and object literal element members are just members | ||
return Navigation.Members; | ||
} | ||
if (ts.isEnumMember(declaration)) { | ||
// enum members are exports | ||
return Navigation.Exports; | ||
} | ||
if ( | ||
ts.isExportSpecifier(declaration) || | ||
ts.isExportAssignment(declaration) || | ||
ts.isExportSpecifier(declaration) || | ||
ts.isExportDeclaration(declaration) || | ||
ts.isNamedExports(declaration) | ||
) { | ||
return Navigation.Exports; | ||
// If it's from an external library, then use an Exports navigation. | ||
if (sourceFile && this._collector.program.isSourceFileFromExternalLibrary(sourceFile)) { | ||
return Navigation.Exports; | ||
} | ||
|
||
// Otherwise, this symbol is from the current package. | ||
if (parent) { | ||
// If we've found an exported CollectorEntity, then it's exported from the package entry point, so | ||
// use an Exports navigation. | ||
const namedDeclaration: ts.DeclarationName | undefined = ( | ||
declaration as ts.NamedDeclaration | undefined | ||
)?.name; | ||
if (namedDeclaration && ts.isIdentifier(namedDeclaration)) { | ||
const collectorEntity: CollectorEntity | undefined = | ||
this._collector.tryGetEntityForNode(namedDeclaration); | ||
octogonz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (collectorEntity && collectorEntity.exported) { | ||
return Navigation.Exports; | ||
} | ||
} | ||
// declarations are exports if they have an `export` modifier. | ||
if (ts.getCombinedModifierFlags(declaration) & ts.ModifierFlags.Export) { | ||
|
||
// If its parent symbol is not a source file, then use an Exports navigation. If the parent symbol is | ||
// a source file, but it wasn't exported from the package entry point (in the check above), then the symbol | ||
// is a local, so fall through below. | ||
if (!DeclarationReferenceGenerator._isExternalModuleSymbol(parent)) { | ||
return Navigation.Exports; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my testing, I could not find a scenario in which any line from 112 to 157 was hit. I could also not find a situation where Maybe this logic was here because previously this method was used to generate the canonical references for API items in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rbuckton any idea what kind of type declaration input would cover these these code paths? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @octogonz shared some instances where |
||
if (ts.isSourceFile(declaration.parent) && !ts.isExternalModule(declaration.parent)) { | ||
// declarations in a source file are global if the source file is not a module. | ||
return 'global'; | ||
} | ||
} | ||
// all other declarations are locals | ||
|
||
// Otherwise, we have a local symbol, so use a Locals navigation. These are either: | ||
// | ||
// 1. Symbols that are exported from a file module but not the package entry point. | ||
// 2. Symbols that are not exported from their parent module. | ||
return Navigation.Locals; | ||
} | ||
|
||
|
@@ -218,20 +186,21 @@ export class DeclarationReferenceGenerator { | |
meaning: ts.SymbolFlags, | ||
includeModuleSymbols: boolean | ||
): DeclarationReference | undefined { | ||
const declaration: ts.Node | undefined = TypeScriptHelpers.tryGetADeclaration(symbol); | ||
const sourceFile: ts.SourceFile | undefined = declaration?.getSourceFile(); | ||
|
||
let followedSymbol: ts.Symbol = symbol; | ||
if (followedSymbol.flags & ts.SymbolFlags.ExportValue) { | ||
followedSymbol = this._typeChecker.getExportSymbolOfSymbol(followedSymbol); | ||
followedSymbol = this._collector.typeChecker.getExportSymbolOfSymbol(followedSymbol); | ||
} | ||
if (followedSymbol.flags & ts.SymbolFlags.Alias) { | ||
followedSymbol = this._typeChecker.getAliasedSymbol(followedSymbol); | ||
followedSymbol = this._collector.typeChecker.getAliasedSymbol(followedSymbol); | ||
} | ||
|
||
if (DeclarationReferenceGenerator._isExternalModuleSymbol(followedSymbol)) { | ||
if (!includeModuleSymbols) { | ||
return undefined; | ||
} | ||
const declaration: ts.Node | undefined = TypeScriptHelpers.tryGetADeclaration(symbol); | ||
const sourceFile: ts.SourceFile | undefined = declaration?.getSourceFile(); | ||
return new DeclarationReference(this._sourceFileToModuleSource(sourceFile)); | ||
} | ||
|
||
|
@@ -270,13 +239,11 @@ export class DeclarationReferenceGenerator { | |
} | ||
} | ||
|
||
let navigation: Navigation | 'global' = | ||
DeclarationReferenceGenerator._getNavigationToSymbol(followedSymbol); | ||
if (navigation === 'global') { | ||
if (parentRef.source !== GlobalSource.instance) { | ||
parentRef = new DeclarationReference(GlobalSource.instance); | ||
} | ||
navigation = Navigation.Exports; | ||
const navigation: Navigation = this._getNavigationToSymbol(followedSymbol); | ||
|
||
// If the symbol is a global, ensure the source is global. | ||
if (sourceFile && !ts.isExternalModule(sourceFile) && parentRef.source !== GlobalSource.instance) { | ||
parentRef = new DeclarationReference(GlobalSource.instance); | ||
} | ||
|
||
return parentRef | ||
|
@@ -313,7 +280,7 @@ export class DeclarationReferenceGenerator { | |
if (grandParent && ts.isModuleDeclaration(grandParent)) { | ||
const grandParentSymbol: ts.Symbol | undefined = TypeScriptInternals.tryGetSymbolForDeclaration( | ||
grandParent, | ||
this._typeChecker | ||
this._collector.typeChecker | ||
); | ||
if (grandParentSymbol) { | ||
return this._symbolToDeclarationReference( | ||
|
@@ -334,28 +301,27 @@ export class DeclarationReferenceGenerator { | |
} | ||
|
||
private _getPackageName(sourceFile: ts.SourceFile): string { | ||
if (this._program.isSourceFileFromExternalLibrary(sourceFile)) { | ||
const packageJson: INodePackageJson | undefined = this._packageJsonLookup.tryLoadNodePackageJsonFor( | ||
sourceFile.fileName | ||
); | ||
if (this._collector.program.isSourceFileFromExternalLibrary(sourceFile)) { | ||
const packageJson: INodePackageJson | undefined = | ||
this._collector.packageJsonLookup.tryLoadNodePackageJsonFor(sourceFile.fileName); | ||
|
||
if (packageJson && packageJson.name) { | ||
return packageJson.name; | ||
} | ||
return DeclarationReferenceGenerator.unknownReference; | ||
} | ||
return this._workingPackageName; | ||
return this._collector.workingPackage.name; | ||
} | ||
|
||
private _sourceFileToModuleSource(sourceFile: ts.SourceFile | undefined): GlobalSource | ModuleSource { | ||
if (sourceFile && ts.isExternalModule(sourceFile)) { | ||
const packageName: string = this._getPackageName(sourceFile); | ||
|
||
if (this._bundledPackageNames.has(packageName)) { | ||
if (this._collector.bundledPackageNames.has(packageName)) { | ||
// The api-extractor.json config file has a "bundledPackages" setting, which causes imports from | ||
// certain NPM packages to be treated as part of the working project. In this case, we need to | ||
// substitute the working package name. | ||
return new ModuleSource(this._workingPackageName); | ||
return new ModuleSource(this._collector.workingPackage.name); | ||
} else { | ||
return new ModuleSource(packageName); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran your build of API Extractor on some monorepo projects and compared the resulting .api.json files with the baseline output. Here's some differences:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I've deleted some replies, since I was comparing in the wrong direction 😆 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. Interesting case with inline type destructuring
This is NOT good API practices. 😄 But it is an interesting edge case:
forgotten-exports.d.ts
index.d.ts
Before this PR, the excerpt for
example1
includes:"canonicalReference": "the-package!IOptions#headers"
After this PR:
"canonicalReference": "the-package!IOptions.headers"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2. Case involving "typeof"
Before this PR:
After this PR:
"canonicalReference": "!Function.prototype:member"
I'm not sure either string is really correct. 😋
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These examples are really useful! I couldn't find any examples of code generating
Navigation.Members
navigation steps. I'll take a look at these. 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3. Case involving preact
This is not a bug really, more of a curiosity:
Before this PR:
After this PR:
It is an improvement, but I think this canonical reference maybe should not have the extra
preact
part?The
Context
is declared like this:[email protected]\node_modules\preact\src\index.d.ts
So it is a member of
namespace preact
but the intended way of importing is reallyimport { Context } from 'preact';
. 🤷♂️There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Across >30 libraries, these were the only weird cases. There were a number of canonical references that are fixed by your PR. Otherwise I didn't see any obvious regressions. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I think cases 1 and 2 should now have the same behavior as before this PR. Previously, I didn't think it was possible for a members navigation step to be present in an identifier passed to this function, but the cases you shared demonstrated it is possible.
I didn't tackle case 3 because this PR is an improvement, and it seemed like potentially a larger fix. But I agree that the currently behavior isn't necessarily ideal.