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

Merged
merged 9 commits into from
Sep 2, 2022

Conversation

zelliott
Copy link
Contributor

@zelliott zelliott commented Jul 26, 2022

Summary

Fixes #3513.

Details

  • Adds apiReport.includeForgottenExports and docModel.includeForgottenExports configs to api-extractor.json that control whether forgotten exports are included in the API report and doc model files, respectively.
  • Adds a new ApiExportedMixin mixin to api-extractor-model to allow API items to store whether they are exported or not from their container module (i.e. entry point or namespace). This is necessary to build the proper canonical reference (i.e. with or without a ~ navigation step) from the .api.json. Note that this mixin doesn't actually serialize the new isExported property to the .api.json because this info is already present in the canonical reference, thus no .api.json change needed.
  • Reworks CollectionEntity and the parts of the Collector that construct them to change the definitions of "consumable" and "exported" as previously defined by api-extractor. More on this below.

Consumable vs exported

Before this PR, collector entities could be exported and/or consumable. An exported entity was a top-level export, like:

/** @public */
export declare class A {}

whereas a consumable entity was either a top-level export OR an entity exported from an AstNamespaceImport. An example of the latter might be:

declare class B {}

/** @public */
export declare namespace n {
  export {
    B
  }
}

This previous definition of consumable is incomplete and misleading for a few reasons:

  1. It doesn't actually care whether the AstNamespaceImport n is exported. This is a bug. This PR fixes that bug.
  2. It only applies to AstNamespaceImport namespaces, not normal AstSymbol namespaces. There's fundamentally no reason why the two should be treated differently. This also leads to bugs. This PR doesn't update AstSymbol namespaces to be treated similarly to AstNamespaceImports, but reworks CollectionEntity and the parts of the Collector that construct them in order to pave the way for a future PR to do this.

This PR changes the definition of consumable to the following: A consumable entity is either a top-level export or an entity exported from a consumable parent namespace.

This PR also updates the definition of exported. The previous definition is not particularly useful today (almost all code paths use consumable instead of exported). Instead, we need to know whether an entity is exported with respect to its parent module, for the purposes of writing the proper navigation step in api-extractor-model. Thus, this PR updates the definition of exported to the following: An exported entity is an entity exported from its parent API item.

Ambiguous IDs

As discussed in #3513, unexported API items will encounter an instance of the "ae-ambiguous-ids" problem. In short, it's possible for there to be multiple unexported API items with the same name that all need to be written to the .api.json. Consider the following:

// foo.ts

type SomeType = number;
export function foo(): SomeType { return 5; }

// bar.ts

type SomeType = number;
export function bar(): SomeType { return 5; }

// index.ts (entry point)

export { foo } from './foo';
export { bar } from './bar';

foo.ts:SomeType and bar.ts:SomeType are both forgotten exports and will be written to the doc model. if docModel.includeForgottenExports is enabled. What names and canonical references do we give these items?

This PR reuses the existing logic in Collector._makeUniqueNames to assign a unique nameForEmit for each item. In practice, one SomeType will keep it's name, and the other will be de-duped to SomeType_2. These unique names will be used when writing both the items' names and canonical references to the doc model. The includeForgottenExports test case tests this scenario.

There are a few important things to note about this:

  1. This is not a new problem. This existing behavior exists today if an exported item's name conflicts with a global name. The exported item will be renamed (e.g. see the ambientNameConflict2 test case, Date and Date_2). This PR uses this same solution.
  2. These names are not stable, names can be shuffled if new items with the same name are introduced. But this can be solved in the future with the @label tag as discussed in [api-extractor] Option to include un-exported types in doc model output #3513.

Known bugs/missing features

This is a list of some known bugs and missing features associated with this PR. If anything in this list should block this PR being submitted, please let me know!

  1. Canonical references in reference tokens (constructed via DeclarationReferenceGenerator) use the item's original name, not it's unique nameForEmit name. I didn't include this fix in this PR because this is an existing bug.
  2. Forgotten exports within consumable namespaces are included in the .api.json and .api.md even if the includeForgottenExports setting is off ([api-extractor] Add options to include forgotten exports in API report and doc model files #3552 (comment)). I didn't include this fix in this PR because this is an existing bug.
  3. @link and @inheritDoc references to unexported API items cannot be resolved. I didn't include this fix in this PR because it seemed like a a relatively minor missing feature and easy to fix as a follow-on task.

How it was tested

I added a new test to api-extractor-scenarios and manually validated that stuff looked correct.

Comment on lines 833 to 838
// 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

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

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.

Comment on lines -12 to -15
// @public (undocumented)
class ForgottenClass {
}

Copy link
Contributor Author

@zelliott zelliott Jul 26, 2022

Choose a reason for hiding this comment

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

We want this change. ForgottenClass is exported by forgottenNs but forgottenNs isn't actually exported itself. Thus, ForgottenClass isn't exported and thus shouldn't appear in the API report. Previously ForgottenClass was marked as consumable when it in fact was not (this bug is mentioned in the PR description).

"members": [
{
"kind": "Class",
"canonicalReference": "api-extractor-scenarios!~ForgottenExport4~ForgottenExport5:class",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This canonical reference should be api-extractor-scenarios!~ForgottenExport4.ForgottenExport5:class, because ForgottenExport5 is exported from ForgottenExport4. However, unfortunately ApiExportedMixin.isExported is false for ForgottenExport5, because ForgottenExport5 determines its exported state from ForgottenExport4's CollectorEntity (i.e. it doesn't have its own CollectorEntity).

This is fundamentally an instance of the pre-existing bug 2 mentioned in the PR description.

{
"kind": "Reference",
"text": "DuplicateName",
"canonicalReference": "api-extractor-scenarios!~DuplicateName:type"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This canonical reference should be api-extractor-scenarios!~DuplicateName_2:type. This is the pre-existing bug 1 mentioned in the PR description.

{
"kind": "Reference",
"text": "ForgottenExport6",
"canonicalReference": "api-extractor-scenarios!ForgottenExport6:class"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Observe that the canonical reference for ForgottenExport6 as determined from the .api.json above is api-extractor-scenarios!internal2.ForgottenExport6:class. This is an existing bug with AstNamespaceImports, discussed a bit in #3584 (comment).

prop?: ForgottenExport2;
}

// Warning: (ae-unresolved-inheritdoc-reference) The @inheritDoc reference could not be resolved: The package "api-extractor-scenarios" does not have an export "ForgottenExport1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is bug 3 mentioned in the PR description.

@zelliott zelliott marked this pull request as ready for review August 15, 2022 15:56
@octogonz
Copy link
Collaborator

👍 I tried running this on a bunch of monorepo projects with apiReport.includeForgottenExports=false and docModel.includeForgottenExports=false and the output was unchanged. It doesn't prove the feature works, but it gives some confidence that the PR didn't introduce any regressions.

# Conflicts:
#	build-tests/install-test-workspace/workspace/common/pnpm-lock.yaml
@octogonz octogonz merged commit 3e0a011 into microsoft:main Sep 2, 2022
@octogonz
Copy link
Collaborator

octogonz commented Sep 2, 2022

🚀 This feature was released with API Extractor 7.30.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[api-extractor] Option to include un-exported types in doc model output
2 participants