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 a bug where an item is exported from both the entry point and an exported AstNamespaceImport. #3619

Merged
merged 5 commits into from
Sep 12, 2022

Conversation

zelliott
Copy link
Contributor

@zelliott zelliott commented Sep 8, 2022

Summary

This PR fixes a minor bug introduced in #3552.

Details

Suppose you have the following setup:

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

export {Foo, internal}

// internal.ts
export class Foo {}

In the code above, Foo is consumable both from the entry point and from a consumable AstNamespaceImport.

Before #3552

Foo is written twice to the doc model: once under the ApiEntryPoint, once under the ApiNamespace internal. This is probably not the ideal behavior, but it's what users before #3552 expect.

After #3552

When docModel.includeForgottenExports is disabled, Foo is now only written once to the doc model under the ApiNamespace. This is due to some incorrect code within ApiModelGenerator.ts that does not process an entry point entity if the entity has any parent entities. This code is incorrect because I didn't consider entities that are exported both from the entry point and from an AstNamespaceImport. Instead, this logic should only process entities that are exported from the entry point. This PR implements that fix.

Additionally, I fixed a bug that occurs in this scenario when docModel.includeForgottenExports is enabled.

Previously, ApiModelGenerator.ts determined whether an item is exported based upon whether it was exported from any parent module (via CollectorEntity.exported). However, this doesn't work if an item is written multiple times to the doc model. This doesn't work because the item's "exported-ness" needs to be evaluated with respect to its respective ApiItemContainerMixin.

Thus. I updated ApiModelGenerator.ts to pass isExported through the process functions. In doing this, I modified the method signatures of the process functions to be more readable by introducing a context parameter and interface. (because we were passing four arguments into each method).

How it was tested

I added two api-extractor-scenario tests: namespaceImports and namespaceImports2:

  • namespaceImports: A scenario where a class is exported from two AstNamespaceImport namespaces, and both namespaces are exported from the entry point.
  • namespaceImports2: A scenario where a class is exported from both an AstNamespaceImport and the entry point.

These test scenarios are also added in #3602 to validate some declaration reference construction logic. This will make rebasing/coordinating between the two PRs easier.

@@ -373,6 +373,22 @@
}
]
},
{
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 addition is expected. ForgottenExport6 is exported from an un-exported AstNamespaceImport. It will appear twice in the doc model:

  • Once under the un-exported namespace internal2, with the reference api-extractor-scenarios!~internal2.ForgottenExport6:class.
  • Once under the entry point itself, with the reference api-extractor-scenarios!~ForgottenExport6:class.

It appears twice because today for largely the same reason items exported from both the entry point and an exported AstNamespaceImport appear twice today. We can improve this behavior in a follow-up PR.

"changes": [
{
"packageName": "@microsoft/api-extractor",
"comment": "Fix a recent regression where items exported from both the entry point and from an exported namespace appeared only once in the API doc model (GitHub #3619)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@zelliott I updated this change message. Our change log guidelines recommend to use the word "issue" or "regression" instead of "bug".

@octogonz octogonz merged commit fd20ea0 into microsoft:main Sep 12, 2022
@octogonz
Copy link
Collaborator

🚀 Released with @microsoft/[email protected]

Thanks for your patience!

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.

2 participants