Skip to content
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 #3593 Incorrect canonical reference to aliased class in .api.json #3602

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions apps/api-extractor/src/analyzer/AstNamespaceImport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export interface IAstNamespaceImportOptions {
readonly astModule: AstModule;
readonly namespaceName: string;
readonly declaration: ts.Declaration;
readonly symbol: ts.Symbol;
}

/**
Expand Down Expand Up @@ -67,11 +68,17 @@ export class AstNamespaceImport extends AstSyntheticEntity {
*/
public readonly declaration: ts.Declaration;

/**
* The original `ts.SymbolFlags.Namespace` symbol.
*/
public readonly symbol: ts.Symbol;

public constructor(options: IAstNamespaceImportOptions) {
super();
this.astModule = options.astModule;
this.namespaceName = options.namespaceName;
this.declaration = options.declaration;
this.symbol = options.symbol;
}

/** {@inheritdoc} */
Expand Down
3 changes: 2 additions & 1 deletion apps/api-extractor/src/analyzer/ExportAnalyzer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,8 @@ export class ExportAnalyzer {
namespaceImport = new AstNamespaceImport({
namespaceName: declarationSymbol.name,
astModule: astModule,
declaration: declaration
declaration: declaration,
symbol: declarationSymbol
});
this._astNamespaceImportByModule.set(astModule, namespaceImport);
}
Expand Down
14 changes: 14 additions & 0 deletions apps/api-extractor/src/collector/Collector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export class Collector {
AstEntity,
CollectorEntity
>();
private readonly _entitiesBySymbol: Map<ts.Symbol, CollectorEntity> = new Map<ts.Symbol, CollectorEntity>();
fwienber marked this conversation as resolved.
Show resolved Hide resolved

private readonly _starExportedExternalModulePaths: string[] = [];

Expand Down Expand Up @@ -294,6 +295,14 @@ export class Collector {
return undefined;
}

/**
* For a given analyzed ts.Symbol, return the CollectorEntity that it refers to. Returns undefined if it
* doesn't refer to anything interesting.
*/
public tryGetEntityForSymbol(symbol: ts.Symbol): CollectorEntity | undefined {
return this._entitiesBySymbol.get(symbol);
}

/**
* Returns the associated `CollectorEntity` for the given `astEntity`, if one was created during analysis.
*/
Expand Down Expand Up @@ -420,6 +429,11 @@ export class Collector {
entity = new CollectorEntity(astEntity);

this._entitiesByAstEntity.set(astEntity, entity);
if (astEntity instanceof AstSymbol) {
this._entitiesBySymbol.set(astEntity.followedSymbol, entity);
} else if (astEntity instanceof AstNamespaceImport) {
this._entitiesBySymbol.set(astEntity.symbol, entity);
}
this._entities.push(entity);
this._collectReferenceDirectives(astEntity);
}
Expand Down
13 changes: 13 additions & 0 deletions apps/api-extractor/src/collector/CollectorEntity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,19 @@ export class CollectorEntity {
return false;
}

/**
* Return the first consumable parent that exports this entity. If there is none, returns
* `undefined`.
*/
public getFirstExportingConsumableParent(): CollectorEntity | undefined {
fwienber marked this conversation as resolved.
Show resolved Hide resolved
for (const [parent, localExportNames] of this._localExportNamesByParent) {
if (parent.consumable && localExportNames.size > 0) {
return parent;
}
}
return undefined;
}

/**
* Adds a new export name to the entity.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { TypeScriptHelpers } from '../analyzer/TypeScriptHelpers';
import { TypeScriptInternals } from '../analyzer/TypeScriptInternals';
import { Collector } from '../collector/Collector';
import { CollectorEntity } from '../collector/CollectorEntity';
import { AstNamespaceImport } from '../analyzer/AstNamespaceImport';

export class DeclarationReferenceGenerator {
public static readonly unknownReference: string = '?';
Expand Down Expand Up @@ -109,34 +110,27 @@ export class DeclarationReferenceGenerator {
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 Exports.
const namedDeclaration: ts.DeclarationName | undefined = (
declaration as ts.NamedDeclaration | undefined
)?.name;
if (namedDeclaration && ts.isIdentifier(namedDeclaration)) {
const collectorEntity: CollectorEntity | undefined =
this._collector.tryGetEntityForNode(namedDeclaration);
if (collectorEntity && collectorEntity.exported) {
return Navigation.Exports;
}
}

// If its parent symbol is not a source file, then use either Exports or Members. 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)) {
if (
parent.members &&
DeclarationReferenceGenerator._isSameSymbol(parent.members.get(symbol.escapedName), symbol)
) {
return Navigation.Members;
}
// Otherwise, this symbol is from the current package. If we've found an associated consumable
// `CollectorEntity`, then use Exports. We use `consumable` here instead of `exported` because
// if the symbol is exported from a non-consumable `AstNamespaceImport`, we don't want to use
// Exports. We should use Locals instead.
const entity: CollectorEntity | undefined = this._collector.tryGetEntityForSymbol(symbol);
if (entity?.consumable) {
return Navigation.Exports;
}

return Navigation.Exports;
// If its parent symbol is not a source file, then use either Exports or Members. 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 (parent && !DeclarationReferenceGenerator._isExternalModuleSymbol(parent)) {
if (
parent.members &&
DeclarationReferenceGenerator._isSameSymbol(parent.members.get(symbol.escapedName), symbol)
) {
return Navigation.Members;
}

return Navigation.Exports;
}

// Otherwise, we have a local symbol, so use a Locals navigation. These are either:
Expand Down Expand Up @@ -209,6 +203,12 @@ export class DeclarationReferenceGenerator {
}
if (followedSymbol.flags & ts.SymbolFlags.Alias) {
followedSymbol = this._collector.typeChecker.getAliasedSymbol(followedSymbol);

// Without this logic, we end up following the symbol `ns` in `import * as ns from './file'` to
// the actual file `file.ts`. We don't want to do this, so revert to the original symbol.
if (followedSymbol.flags & ts.SymbolFlags.ValueModule) {
followedSymbol = symbol;
}
}

if (DeclarationReferenceGenerator._isExternalModuleSymbol(followedSymbol)) {
Expand All @@ -229,6 +229,11 @@ export class DeclarationReferenceGenerator {
}

let localName: string = followedSymbol.name;
const entity: CollectorEntity | undefined = this._collector.tryGetEntityForSymbol(followedSymbol);
if (entity?.nameForEmit) {
localName = entity.nameForEmit;
}

if (followedSymbol.escapedName === ts.InternalSymbolName.Constructor) {
localName = 'constructor';
} else {
Expand Down Expand Up @@ -267,8 +272,38 @@ export class DeclarationReferenceGenerator {

private _getParentReference(symbol: ts.Symbol): DeclarationReference | undefined {
const declaration: ts.Node | undefined = TypeScriptHelpers.tryGetADeclaration(symbol);
const sourceFile: ts.SourceFile | undefined = declaration?.getSourceFile();

// Note that it's possible for a symbol to be exported from an entry point as well as one or more
// namespaces. In that case, it's not clear what to choose as its parent. Today's logic is neither
// perfect nor particularly stable to API items being renamed and shuffled around.
const entity: CollectorEntity | undefined = this._collector.tryGetEntityForSymbol(symbol);
if (entity) {
if (entity.exportedFromEntryPoint) {
return new DeclarationReference(this._sourceFileToModuleSource(sourceFile));
}

const firstExportingConsumableParent: CollectorEntity | undefined =
entity.getFirstExportingConsumableParent();
if (
firstExportingConsumableParent &&
firstExportingConsumableParent.astEntity instanceof AstNamespaceImport
) {
const parentSymbol: ts.Symbol | undefined = TypeScriptInternals.tryGetSymbolForDeclaration(
firstExportingConsumableParent.astEntity.declaration,
this._collector.typeChecker
);
if (parentSymbol) {
return this._symbolToDeclarationReference(
parentSymbol,
parentSymbol.flags,
/*includeModuleSymbols*/ true
);
}
}
}

// First, try to find a parent symbol via the symbol tree.
// Next, try to find a parent symbol via the symbol tree.
const parentSymbol: ts.Symbol | undefined = TypeScriptInternals.getSymbolParent(symbol);
if (parentSymbol) {
return this._symbolToDeclarationReference(
Expand Down Expand Up @@ -306,7 +341,6 @@ export class DeclarationReferenceGenerator {
}

// At this point, we have a local symbol in a module.
const sourceFile: ts.SourceFile | undefined = declaration?.getSourceFile();
if (sourceFile && ts.isExternalModule(sourceFile)) {
return new DeclarationReference(this._sourceFileToModuleSource(sourceFile));
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
"docReferences",
"docReferences2",
"docReferences3",
"docReferencesAlias",
"docReferencesNamespaceAlias",
"dynamicImportType",
"dynamicImportType2",
"dynamicImportType3",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@
{
"kind": "Reference",
"text": "MyPromise",
"canonicalReference": "api-extractor-scenarios!~Promise:class"
"canonicalReference": "api-extractor-scenarios!~Promise_2:class"
},
{
"kind": "Content",
Expand Down
Loading