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.

Sign up for GitHub

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] Add options to include forgotten exports in API report and doc model files #3552

Merged
merged 9 commits into from
Sep 2, 2022
Merged

[api-extractor] Add options to include forgotten exports in API report and doc model files #3552

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
42ab8f0
WIP
zelliott Jul 26, 2022
d0393b0
Ran rush change
zelliott Jul 26, 2022
f9a98d4
Small bug fix
zelliott Jul 26, 2022
d4e758d
Update PR based upon reviewer feedback
zelliott Aug 1, 2022
84de9f3
Rebased off latest master changes
zelliott Aug 14, 2022
3b2129d
Responded to PR comments and prepared PR for review
zelliott Aug 15, 2022
02ee8a0
Fix subtle bug with ApiNamespaceImports
zelliott Aug 15, 2022
d9a0587
Respond to PR feedback
zelliott Aug 25, 2022
54db4bb
Merge remote-tracking branch 'remotes/origin/main' into exported
octogonz Sep 2, 2022
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
14 changes: 14 additions & 0 deletions apps/api-extractor/src/api/ExtractorConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,10 @@ interface IExtractorConfigParameters {
apiReportEnabled: boolean;
reportFilePath: string;
reportTempFilePath: string;
apiReportIncludeForgottenExports: boolean;
docModelEnabled: boolean;
apiJsonFilePath: string;
docModelIncludeForgottenExports: boolean;
rollupEnabled: boolean;
untrimmedFilePath: string;
alphaTrimmedFilePath: string;
Expand Down Expand Up @@ -246,11 +248,15 @@ export class ExtractorConfig {
public readonly reportFilePath: string;
/** The `reportTempFolder` path combined with the `reportFileName`. */
public readonly reportTempFilePath: string;
/** {@inheritDoc IConfigApiReport.includeForgottenExports} */
public readonly apiReportIncludeForgottenExports: boolean;

/** {@inheritDoc IConfigDocModel.enabled} */
public readonly docModelEnabled: boolean;
/** {@inheritDoc IConfigDocModel.apiJsonFilePath} */
public readonly apiJsonFilePath: string;
/** {@inheritDoc IConfigDocModel.includeForgottenExports} */
public readonly docModelIncludeForgottenExports: boolean;

/** {@inheritDoc IConfigDtsRollup.enabled} */
public readonly rollupEnabled: boolean;
Expand Down Expand Up @@ -307,8 +313,10 @@ export class ExtractorConfig {
this.apiReportEnabled = parameters.apiReportEnabled;
this.reportFilePath = parameters.reportFilePath;
this.reportTempFilePath = parameters.reportTempFilePath;
this.apiReportIncludeForgottenExports = parameters.apiReportIncludeForgottenExports;
this.docModelEnabled = parameters.docModelEnabled;
this.apiJsonFilePath = parameters.apiJsonFilePath;
this.docModelIncludeForgottenExports = parameters.docModelIncludeForgottenExports;
this.rollupEnabled = parameters.rollupEnabled;
this.untrimmedFilePath = parameters.untrimmedFilePath;
this.alphaTrimmedFilePath = parameters.alphaTrimmedFilePath;
Expand Down Expand Up @@ -848,6 +856,7 @@ export class ExtractorConfig {
let apiReportEnabled: boolean = false;
let reportFilePath: string = '';
let reportTempFilePath: string = '';
let apiReportIncludeForgottenExports: boolean = false;
if (configObject.apiReport) {
apiReportEnabled = !!configObject.apiReport.enabled;

Expand Down Expand Up @@ -879,17 +888,20 @@ export class ExtractorConfig {

reportFilePath = path.join(reportFolder, reportFilename);
reportTempFilePath = path.join(reportTempFolder, reportFilename);
apiReportIncludeForgottenExports = !!configObject.apiReport.includeForgottenExports;
}

let docModelEnabled: boolean = false;
let apiJsonFilePath: string = '';
let docModelIncludeForgottenExports: boolean = false;
if (configObject.docModel) {
docModelEnabled = !!configObject.docModel.enabled;
apiJsonFilePath = ExtractorConfig._resolvePathWithTokens(
'apiJsonFilePath',
configObject.docModel.apiJsonFilePath,
tokenContext
);
docModelIncludeForgottenExports = !!configObject.docModel.includeForgottenExports;
}

let tsdocMetadataEnabled: boolean = false;
Expand Down Expand Up @@ -993,8 +1005,10 @@ export class ExtractorConfig {
apiReportEnabled,
reportFilePath,
reportTempFilePath,
apiReportIncludeForgottenExports,
docModelEnabled,
apiJsonFilePath,
docModelIncludeForgottenExports,
rollupEnabled,
untrimmedFilePath,
alphaTrimmedFilePath,
Expand Down
24 changes: 23 additions & 1 deletion apps/api-extractor/src/api/IConfigFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,17 @@ export interface IConfigApiReport {
* prepend a folder token such as `<projectFolder>`.
*/
reportTempFolder?: string;

/**
* Whether "forgotten exports" should be included in the API report file.
*
* @remarks
* Forgotten exports are declarations flagged with `ae-forgotten-export` warnings. See
* https://api-extractor.com/pages/messages/ae-forgotten-export/ to learn more.
*
* @defaultValue `false`
*/
includeForgottenExports?: boolean;
}

/**
Expand All @@ -120,6 +131,17 @@ export interface IConfigDocModel {
* prepend a folder token such as `<projectFolder>`.
*/
apiJsonFilePath?: string;

/**
* Whether "forgotten exports" should be included in the doc model file.
*
* @remarks
* Forgotten exports are declarations flagged with `ae-forgotten-export` warnings. See
* https://api-extractor.com/pages/messages/ae-forgotten-export/ to learn more.
*
* @defaultValue `false`
*/
includeForgottenExports?: boolean;
}

/**
Expand Down Expand Up @@ -376,7 +398,7 @@ export interface IConfigFile {
testMode?: boolean;

/**
* Specifies how API Extractor sorts members of an enum when generating api.json.
* Specifies how API Extractor sorts members of an enum when generating the .api.json file.
*
* @remarks
* By default, the output files will be sorted alphabetically, which is "by-name".
Expand Down
85 changes: 46 additions & 39 deletions apps/api-extractor/src/collector/Collector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,28 +246,23 @@ export class Collector {
this.workingPackage.tsdocComment = this.workingPackage.tsdocParserContext!.docComment;
}

const exportedAstEntities: AstEntity[] = [];

// Create a CollectorEntity for each top-level export

const astModuleExportInfo: AstModuleExportInfo =
this.astSymbolTable.fetchAstModuleExportInfo(astEntryPoint);

// Create a CollectorEntity for each top-level export.
const processedAstEntities: AstEntity[] = [];
for (const [exportName, astEntity] of astModuleExportInfo.exportedLocalEntities) {
this._createCollectorEntity(astEntity, exportName);

exportedAstEntities.push(astEntity);
processedAstEntities.push(astEntity);
}

// Create a CollectorEntity for each indirectly referenced export.
// Note that we do this *after* the above loop, so that references to exported AstSymbols
// are encountered first as exports.
const alreadySeenAstSymbols: Set<AstSymbol> = new Set<AstSymbol>();
for (const exportedAstEntity of exportedAstEntities) {
this._createEntityForIndirectReferences(exportedAstEntity, alreadySeenAstSymbols);

if (exportedAstEntity instanceof AstSymbol) {
this.fetchSymbolMetadata(exportedAstEntity);
// Recursively create the remaining CollectorEntities after the top-level entities
// have been processed.
const alreadySeenAstEntities: Set<AstEntity> = new Set<AstEntity>();
for (const astEntity of processedAstEntities) {
this._recursivelyCreateEntities(astEntity, alreadySeenAstEntities);
if (astEntity instanceof AstSymbol) {
this.fetchSymbolMetadata(astEntity);
}
}

Expand Down Expand Up @@ -414,7 +409,11 @@ export class Collector {
return overloadIndex;
}

private _createCollectorEntity(astEntity: AstEntity, exportedName: string | undefined): CollectorEntity {
private _createCollectorEntity(
astEntity: AstEntity,
exportName?: string,
parent?: CollectorEntity
): CollectorEntity {
let entity: CollectorEntity | undefined = this._entitiesByAstEntity.get(astEntity);

if (!entity) {
Expand All @@ -425,50 +424,54 @@ export class Collector {
this._collectReferenceDirectives(astEntity);
}

if (exportedName) {
entity.addExportName(exportedName);
if (exportName) {
if (parent) {
entity.addLocalExportName(exportName, parent);
} else {
entity.addExportName(exportName);
}
}

return entity;
}

private _createEntityForIndirectReferences(
astEntity: AstEntity,
alreadySeenAstEntities: Set<AstEntity>
): void {
if (alreadySeenAstEntities.has(astEntity)) {
return;
}
private _recursivelyCreateEntities(astEntity: AstEntity, alreadySeenAstEntities: Set<AstEntity>): void {
octogonz marked this conversation as resolved.
Show resolved Hide resolved
if (alreadySeenAstEntities.has(astEntity)) return;
alreadySeenAstEntities.add(astEntity);

if (astEntity instanceof AstSymbol) {
astEntity.forEachDeclarationRecursive((astDeclaration: AstDeclaration) => {
for (const referencedAstEntity of astDeclaration.referencedAstEntities) {
if (referencedAstEntity instanceof AstSymbol) {
// We only create collector entities for root-level symbols.
// For example, if a symbols is nested inside a namespace, only the root-level namespace
// get a collector entity
// We only create collector entities for root-level symbols. For example, if a symbol is
// nested inside a namespace, only the namespace gets a collector entity. Note that this
// is not true for AstNamespaceImports below.
if (referencedAstEntity.parentAstSymbol === undefined) {
this._createCollectorEntity(referencedAstEntity, undefined);
this._createCollectorEntity(referencedAstEntity);
}
octogonz marked this conversation as resolved.
Show resolved Hide resolved
} else {
this._createCollectorEntity(referencedAstEntity, undefined);
this._createCollectorEntity(referencedAstEntity);
}

this._createEntityForIndirectReferences(referencedAstEntity, alreadySeenAstEntities);
this._recursivelyCreateEntities(referencedAstEntity, alreadySeenAstEntities);
}
});
}

if (astEntity instanceof AstNamespaceImport) {
const astModuleExportInfo: AstModuleExportInfo = astEntity.fetchAstModuleExportInfo(this);
const parentEntity: CollectorEntity | undefined = this._entitiesByAstEntity.get(astEntity);
if (!parentEntity) {
// This should never happen, as we've already created entities for all AstNamespaceImports.
throw new InternalError(
`Failed to get CollectorEntity for AstNamespaceImport with namespace name "${astEntity.namespaceName}"`
);
}

for (const exportedEntity of astModuleExportInfo.exportedLocalEntities.values()) {
// Create a CollectorEntity for each top-level export of AstImportInternal entity
const entity: CollectorEntity = this._createCollectorEntity(exportedEntity, undefined);
entity.addAstNamespaceImports(astEntity);

this._createEntityForIndirectReferences(exportedEntity, alreadySeenAstEntities);
for (const [localExportName, localAstEntity] of astModuleExportInfo.exportedLocalEntities) {
// Create a CollectorEntity for each local export within an AstNamespaceImport entity.
this._createCollectorEntity(localAstEntity, localExportName, parentEntity);
this._recursivelyCreateEntities(localAstEntity, alreadySeenAstEntities);
}
}
}
Expand Down Expand Up @@ -826,10 +829,14 @@ export class Collector {
astSymbol
);
}

// All internal, exported APIs default to public if no release tag is specified.
options.effectiveReleaseTag = ReleaseTag.Public;
}
} else {
// All external APIs default to public if no release tag is specified.
options.effectiveReleaseTag = ReleaseTag.Public;
Copy link
Contributor Author

@zelliott zelliott Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This change prevents unexported items included in an API report from being assigned the @public release tag if none is specified.

Copy link
Collaborator

@octogonz octogonz Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

After some discussion, I think this logic is not right. The "effective" release tag must always be something other than ReleaseTag.None.

The simplest approach would be to handle forgotten exports like any other API item:

  • API Extractor complains if it doesn't have a release tag (because for example .d.ts trimming is not going to produce good results if developers don't think carefully about what is/isn't public)
  • If you disable the warnings, then everything defaults to @public (because in this case, the developer presumably doesn't care very much about trimming or release tags)

Note that when includeForgottenExports=false then this validation is not useful, and prior to this PR, API Extractor would not complain about problems with forgotten exports. But with includeForgottenExports=true we probably do want such validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this PR to match the behavior you described above. However, I was thinking a bit more about forgotten exports and release tags and sent you a message on Zulip, reproduced below:

We decided yesterday that the missing release tag warning should only be logged if includeForgottenExports is on. I'm wondering now if the missing release tag warning should be logged regardless of whether that config is on. Why? Because .d.ts trimming will produce better results if forgotten exports have release tags.

Maybe the original thinking was that a developer will already get an ae-forgotten-export warning for these items, and so there's already "some warning" indicating that these items should be exported (and thus also should have release tags). But (1) what if the developer turned off that warning and (2) even if the developer didn't, it still might be useful to report both warnings (otherwise a developer might slap on an export, re-run API Extractor, and then just hit another warning right after, as they forgot to also add a release tag).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, but I think we could implement that extra validation in a separate PR.

}

options.effectiveReleaseTag = ReleaseTag.Public;
}

const apiItemMetadata: ApiItemMetadata = new ApiItemMetadata(options);
Expand Down
Loading