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 5 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
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,7 @@ dist

# Heft
.heft

# IntelliJ IDEA
.idea
*.iml
37 changes: 37 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 @@ -301,6 +302,37 @@ export class Collector {
return this._entitiesByAstEntity.get(astEntity);
}

/**
* Returns the name to emit for the given `symbol`.
* This is usually the `name` of that `symbol`.
* Only if this `symbol` has a `CollectorEntity`, the `nameForEmit` of that entity is used.
* @param symbol the symbol to compute the name to emit for
* @returns the name to emit for the given `symbol`
*/
public getEmitName(symbol: ts.Symbol): string {
const collectorEntity: CollectorEntity | undefined = this._entitiesBySymbol.get(symbol);
return collectorEntity?.nameForEmit ?? symbol.name;
}

/**
* Returns (the symbol of) the namespace that exports the given `symbol`, if applicable.
* @param symbol the symbol representing the namespace that exports the given `symbol`,
* or undefined if there is no such namespace
*/
public getExportingNamespace(symbol: ts.Symbol): ts.Symbol | undefined {
const collectorEntity: CollectorEntity | undefined = this._entitiesBySymbol.get(symbol);
if (collectorEntity) {
const exportingNamespace: AstNamespaceImport | undefined = collectorEntity.getExportingNamespace();
if (exportingNamespace) {
return TypeScriptInternals.tryGetSymbolForDeclaration(
exportingNamespace.declaration,
this.typeChecker
);
}
}
return undefined;
}

public fetchSymbolMetadata(astSymbol: AstSymbol): SymbolMetadata {
if (astSymbol.symbolMetadata === undefined) {
this._fetchSymbolMetadata(astSymbol);
Expand Down Expand Up @@ -420,6 +452,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.declaration as unknown as ts.Type).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 @@ -7,6 +7,7 @@ import { AstSymbol } from '../analyzer/AstSymbol';
import { Collector } from './Collector';
import { Sort } from '@rushstack/node-core-library';
import { AstEntity } from '../analyzer/AstEntity';
import { AstNamespaceImport } from '../analyzer/AstNamespaceImport';

/**
* This is a data structure used by the Collector to track an AstEntity that may be emitted in the *.d.ts file.
Expand Down Expand Up @@ -189,6 +190,18 @@ export class CollectorEntity {
return this._localExportNamesByParent.size > 0;
}

/**
* Return the first namespace that exports this entity.
*/
public getExportingNamespace(): AstNamespaceImport | undefined {
for (const [parent, localExportNames] of this._localExportNamesByParent) {
if (parent.consumable && localExportNames.size > 0 && parent.astEntity instanceof AstNamespaceImport) {
return parent.astEntity;
}
}
return undefined;
}

/**
* Adds a new export name to the entity.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,18 @@ export class DeclarationReferenceGenerator {
}

private _getNavigationToSymbol(symbol: ts.Symbol): Navigation {
// resolve alias first:
if (symbol.flags & ts.SymbolFlags.Alias) {
symbol = this._collector.typeChecker.getAliasedSymbol(symbol);
}
// namespace always uses ".":
if (symbol.flags & ts.SymbolFlags.Namespace) {
return Navigation.Exports;
}
const declaration: ts.Declaration | undefined = TypeScriptHelpers.tryGetADeclaration(symbol);
const sourceFile: ts.SourceFile | undefined = declaration?.getSourceFile();
const parent: ts.Symbol | undefined = TypeScriptInternals.getSymbolParent(symbol);
const parent: ts.Symbol | undefined =
TypeScriptInternals.getSymbolParent(symbol) ?? this._collector.getExportingNamespace(symbol);

// If it's global or from an external library, then use either Members or Exports. It's not possible for
// global symbols or external library symbols to be Locals.
Expand Down Expand Up @@ -208,7 +217,11 @@ export class DeclarationReferenceGenerator {
followedSymbol = this._collector.typeChecker.getExportSymbolOfSymbol(followedSymbol);
}
if (followedSymbol.flags & ts.SymbolFlags.Alias) {
followedSymbol = this._collector.typeChecker.getAliasedSymbol(followedSymbol);
const nextFollowedSymbol: ts.Symbol = this._collector.typeChecker.getAliasedSymbol(followedSymbol);
// do not follow alias to namespace, as it results in a source file that no longer knows its short name:
if (!(nextFollowedSymbol.flags & ts.SymbolFlags.Namespace)) {
followedSymbol = nextFollowedSymbol;
}
}

if (DeclarationReferenceGenerator._isExternalModuleSymbol(followedSymbol)) {
Expand All @@ -228,7 +241,7 @@ export class DeclarationReferenceGenerator {
return undefined;
}

let localName: string = followedSymbol.name;
let localName: string = this._collector.getEmitName(followedSymbol);
if (followedSymbol.escapedName === ts.InternalSymbolName.Constructor) {
localName = 'constructor';
} else {
Expand Down Expand Up @@ -266,9 +279,13 @@ export class DeclarationReferenceGenerator {
}

private _getParentReference(symbol: ts.Symbol): DeclarationReference | undefined {
const declaration: ts.Node | undefined = TypeScriptHelpers.tryGetADeclaration(symbol);
// First case: the symbol is exported via a namespace
const exportingNamespace: ts.Symbol | undefined = this._collector.getExportingNamespace(symbol);
if (exportingNamespace) {
return this._symbolToDeclarationReference(exportingNamespace, exportingNamespace.flags, true);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if a symbol is exported from both a namespace and the entry point? Consider the following:

// index.ts
import * as i1 from './internal';
import { SomeClass } from './internal';

export { i1, SomeClass }

// internal.ts
export class SomeClass {}

What should the declaration reference of SomeClass be? Today, API Extractor will actually write two items to the doc model for SomeClass:

  • my-package!SomeClass:class as a child of the ApiEntryPoint.
  • my-package!i1.SomeClass:class as a child of the ApiNamespace my-package!i1.

(Note that today API Extractor will actually write only the latter to the doc model, but this is a recent bug from #3552 and relatively trivial to fix. I'm working on it.)

Ideally API Extractor would only write SomeClass once to the doc model... so which declaration reference is right? Today, your PR generates my-package!i1.SomeClass:class for reference tokens, in part due to the logic highlighted by this comment.

Another situation - suppose a symbol is exported from multiple namespaces, in some convoluted way like this:

// index.ts
import * as i1 from './intermediate1';
import * as i2 from './intermediate2';
export { i1, i2 }

// intermediate1.ts
import * as internal from './internal';
export { internal }

// intermediate2.ts
import * as internal from './internal';
export { internal }

// internal.ts
export class SomeClass {}

What should the declaration reference of SomeClass be? Again, today, API Extractor writes two items for SomeClass:

  • my-package!i1.internal.SomeClass
  • my-package!i2.internal.SomeClass

And today, your PR is indeterministic in which one it generates for reference tokens.


I think fundamentally this is another instance of #1308. I'm not sure what the right behavior is. But I do think we want the behavior to be somewhat deterministic and robust to shuffling of API items. One idea is to choose the shortest ID, and among IDs of the same length, choose the one that comes first alphabetically. That being said, I'm not sure if this open question should block this PR.

@octogonz - what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a bad idea in the first place to expose the same module under two different names. However, if you want to support this use case, maybe the developer should have some means to specify which alias is the "canonical" one and thus has precedence. Could be a new api-extractor configuration or even a new TSDoc tag.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One idea is to choose the shortest ID, and among IDs of the same length, choose the one that comes first alphabetically.

This sounds good to me. Maybe we could also provide a TSDoc tag that allows the choice to be explicitly specified, similar to suggestions for the other ae-ambiguous-ids issues.


// 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 All @@ -290,6 +307,7 @@ export class DeclarationReferenceGenerator {
//
// In the example above, `SomeType` doesn't have a parent symbol per the TS internal API above,
// but its reference still needs to be qualified with the parent reference for `n`.
const declaration: ts.Node | undefined = TypeScriptHelpers.tryGetADeclaration(symbol);
const grandParent: ts.Node | undefined = declaration?.parent?.parent;
if (grandParent && ts.isModuleDeclaration(grandParent)) {
const grandParentSymbol: ts.Symbol | undefined = TypeScriptInternals.tryGetSymbolForDeclaration(
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