[api-extractor-model] Add new ApiItemContainerMixin.findMembersWithInheritance method

[zelliott]

Summary

This PR includes the api-extractor-model changes necessary for addressing #3429. All api-documenter changes for actually showing the inherited members as well as testing this change are in the child PR zelliott#3.

Details

Changes include

• Added a new method ApiItemContainerMixin. findMembersWithInheritance which is the meat of this PR. This method walks an item's inheritance tree and finds all immediate and inherited members. It has comprehensive comments explaining its behavior and various edge cases. I'll also go into various scenarios later in this PR description.
• Added a new IFindApiItemsResult type that can be used as a generic return type for future "find operations" performed by api-extractor-model.
• Added api-extractor-scenarios tests for mixin patterns and merged declarations. These tests aren't strictly needed for this PR, and they're not particularly supported by API Extractor as-is, but I found them useful in validating API Extractor's current behavior. Happy to remove them or move them to a different PR.

Support levels for various types of inheritance

There are many different flavors of inheritance in TypeScript and this PR does not support all of them. Here is this PR's support level by inheritance scenario:

Full support

1. Basic class/interface inheritance (e.g. class A extends B or interface A extends B)
2. Multiple interface inheritance (e.g. interface A extends B, C)

Partial support

1. Class inheritance with type arguments (e.g. class A extends B<number>). Types of inherited members are relative to their defining class, not inherited class.
2. Class/interface inheriting from merged declaration. Members inherited from one of the merged declarations will be missing.

No support

1. Class/interface inheriting from unexported class/interface. Not supported as unexported items aren't present in the .api.json. Will be supported in [api-extractor] Better support for handling unexported base classes #3430.
2. Class/interface inheriting from class/interface not present in .api.json (e.g. due to being in an external, third-party package).
3. Class inheriting from class-like const variable. Not supported as variables don't have members.
4. Class inheriting from anonymous class. Not supported as anonymous classes are (1) unexported and (2) represented in the d.ts as const variables.
5. Interface inheriting from interface-like type alias. Not supported as type aliases don't have members.

Open questions

I'd expect these questions to be resolved before transitioning this PR from draft --> ready:

1. Are we okay with the behavior as documented above in the various scenarios (i.e. full/partial/no support)? Answer: Yes.
2. Is findMembersWithInheritance the right approach here or are we essentially "re-implementing inheritance" and we're better off using the type checker in api-extractor to encode the appropriate inheritance information to the API model Answer: @octogonz and I decided this is the right approach for now and can be extended to support more types of inheritance in the future
3. What's the best way of testing this change? It's not possible to test the change using the api-documenter-test project as-is, as the showInheritedMembers config setting is only available via the api-documenter generate workflow, and the test project uses the api-documenter markdown workflow. It's non trivial to change from generate to markdown. I've manually tested the change on my end by manually and it looks good, but I'd like actual checked in artifacts. Answer: Will add a new api-documenter-scenarios test project in a child PR.
4. Should we also make any changes to YamlDocumenter? Answer: No.
5. Note that this PR is also blocked by the bug fix at [api-documenter] Write protected and readonly modifier information to relevant API item tables #3483. Answer: No longer blocked.

How it was tested

Tested in the child PR zelliott#3.

[octogonz]

(Let's focus on getting PRs 3483, 3484, 3435 merged first, then remind me and I'll take a look at this.)

[octogonz]

• Are we okay with the behavior as documented above in the various scenarios (i.e. full/partial/no support)?

It sounds okay to me. The most interesting case is:

Class/interface inheriting from class/interface not present in .api.json (e.g. due to being in an external, third-party package).

This design choice focuses on having documentation pages (API items) for inherited items.

We might contrast with a design that focuses on showing every possible item even if we can't document it: For each API item, API Extractor might use the compiler engine to collect every inherited member, and add supplementary trivia fields to .api.json with the (package, container, member, type). This way, even if you don't have an .api.json file for the referenced library, API Documenter could still show very basic information like:

property	type	description
colors	Map<string, Color>	Regular member of this class
title	string	(inherited from Component) Docs for this normal member
context	IContext | undefined	(inherited from BaseThing in external-lib)

package="external-lib", container="BaseThing", member="context", type="IContext | undefined"

If external-lib.api.json is available, then API Documenter would use that to generate a normal row. But if not, then it could at least fill in the this basic trivia. The main downside would be adding a lot of bloat to .api.json since potentially every API Item would need to include the full set of trivia items.

🤔 And I suppose there's a counterargument that, if an API is actually important to end users, then we should find a way to define an .api.json file for it. In fact maybe it is actually a better approach to provide a way to generate .api.json files for external libraries. (?)

In any case, such a feature could be a complementary to what you're implementing in this PR.

• Is resolveInheritedMembers the right approach here or are we essentially "re-implementing inheritance" and we're better off using the type checker in api-extractor to encode the appropriate inheritance information to the API model?

I think I agree with ApiExtendsMixin and the idea that .api.json provides sufficient information to work out the hierarchy. If there is something brittle about traversing ExcerptToken's and resolving canonical references, we should try to fix that in the model.

The most interesting gap would be something like:

package-a

export interface A { }

package-b

export interface B extends A { }

package-c

export interface C extends B { }

...where we generate a.api.json and c.api.json but NOT b.api.json. (But from the above reflections, maybe generating b.api.json is the right way to deal with that.)

• What's the best way of testing this change? It's not possible to test the change using the api-documenter-test project as-is, as the showInheritedMembers config setting is only available via the api-documenter generate workflow, and the test project uses the api-documenter markdown workflow. It's non trivial to change from generate to markdown. I've manually tested the change on my end by manually and it looks good, but I'd like actual checked in artifacts.

Ideally we should make an api-documenter-scenarios project with runScenarios.ts driven by theapi-documenter.json config files, one file for each test. If that's too much work, maybe instead fork api-documenter-test into api-documenter-test-00 and api-documenter-test-01.

• Should we also make any changes to YamlDocumenter?

I seem to remember that DocFX itself already has a way to include inherited members. (?)

It's best not to modify YamlDocumenter unless you have a pipeline available to test it. Microsoft's pipelines are unfortunately internal only, so we have to ask one of their product teams to test it. @rbuckton found a way to run the open source DocFX to generate his esfx docs, but last time I looked I wasn't able to figure out what command to run to regenerate the ESFX docs. (?)

[octogonz]

(speaking of which, when will this config-based invocation be out of beta and officially documented?).

As soon as anyone is available to do that work. The api-documenter.json config file seems like the right way to go. The CLI front end for API Documenter is pretty basic, so it doesn't seem like a lot of work to make the config file the primary/recommended way to invoke API Documenter.

[zelliott]

This code was previously only called for ApiInterface items, I think this was a bug? Now it's called for both ApiClass and ApiInterface items.

[octogonz]

This was introduced by PR #1317. The PR notes say that "type parameters are supported by docfx" but doesn't give any reason why the YAML is only populated for interfaces.

@rbuckton was this simply an oversight? If we start emitting type parameters for classes in DocFX YAML, is there any risk of a regression?

[zelliott]

I reverted this change from this PR because it's no longer really related (I'm not even touching YamlDocumenter.ts anymore. It'd still be nice to fix this, it looks like an oversight, I'll open a separate PR.

[zelliott]

Opened #3546.

[zelliott]

In a future PR, I'd like to never write empty token ranges to API doc models.

[octogonz]

The serializer currently takes the approach of always writing every field, even if the contents are empty. The original motivation was to make the JSON file format somewhat self-documenting, such that someone could write a correct loader for it without having to look at the source code. It seemed more important back then, because there was no api-extractor-model library to validate the deserialization, just a JSON schema and some TypeScript interfaces to access the JSON objects.

We could consider a different approach of trying to make the .api.json files as small as possible. But that would only make sense if we applied it across all fields, not just the token ranges.

[zelliott]

Okay @octogonz, I think I've responded to all of your open comments on this PR. I've also pulled out all api-documenter changes necessary for actually showing inherited members into a separate child branch. This child branch includes:

• A new showInheritedMembers api-documenter config (only supported by the MarkdownDocumenter at the moment).
• A new api-documenter-scenarios project. 🎉
• A new "inheritedMembers" test scenario that tests this PR as well as the child branch.
• Significant nit cleanups to the api-documenter project.
• Some nit cleanups to the api-extractor-scenarios project.

How do we want to go about reviewing and merging these branches/PRs?

[octogonz]

How do we want to go about reviewing and merging these branches/PRs?

Let's make a second PR for inheritance-docs-api-documenter

[zelliott]

Created zelliott#3.

[octogonz]

missingData? incompleteResult?

If I saw maybeIncompleteMembers=true without any docs, from the name alone I'm not sure it would be obvious that a problem occurred, or that Members is referring to the FindMembersWithInheritanceResult.members output. (Maybe I'm being obtuse though heheh.)

[zelliott]

Renamed to maybeIncompleteResult, let me know if you'd prefer incompleteResult. My thought behind "maybe" is that it's possible that even if an error is hit finding inherited members, the inherited members are still complete. Consider the following:

type A = {}
interface B extends A {}

findMembersWithInheritance doesn't support finding inherited members from type aliases like A, but fortunately in this case it doesn't really matter, as A has no members.

[zelliott]

Okay, I think this PR is ready for another look. Some open questions:

1. We have a new IFindApiItemsResult object that is currently used by findMembersWithInheritance and will be used for future "find operations". It'd be nice to align on the "find" method name prefix for all operations that return a IFindApiItemsResult object, but there are two existing methods today that use this prefix and just return a readonly list of API items (i.e. findMembersByName and findEntryPointsByPath). Might we want to rename these methods to getMembersByName and getEntryPointsByPath instead? It could be a separate PR.
2. Do we like IFindApiItemsResult or IFindApiItemResult? Future find operations may return multiple API items or only a single API item.
3. I'd like some special attention given to the IFindApiItemMessages. I imagine that these messages need to be generic across all "find operations", so I avoided mentioning "inherited members" in any of them.

[octogonz]

@zelliott I moved your doc comment to interface ApiItemContainerMixin which is the publicly visible signature. (The class MixedClass declaration is inside a local function scope.)

[octogonz]

I don't understand this logic. The condition extendsTypes === undefined means that the current thing is not a class or an interface; it is some other container such as ApiEnum. If I call apiEnum.findMembersWithInheritance(), I would expect it to return the enum members. The fact that there aren't any "inherited" members seems unimportant. Whereas this message seems to imply that there is some kind of problem: "Item apiEnum is of unsupported kind Enum"

[zelliott]

The fact that there aren't any "inherited" members seems unimportant.

Hm, I suppose I was thinking that if someone calls apiEnum.findMembersWithInheritance() the message might be useful in communicating essentially "Hey, just so you know, you asked for inherited members, but ApiEnums do not support inheritance". But I can see your point of view, so I'm happy to add a check at the top of this method that basically just defers to get members() if the API item is neither an ApiClass or ApiInterface.

[zelliott]

I don't feel strongly either way though, so I changed it in #3545 to just return the members without any messages or incompleteness.

[octogonz]

!firstReferenceToken occurs if the excerpt does not contain any token with a canonicalReference. The analyzer can generate canonical references for a very wide variety of declarations, including things that don't get an ApiItem, so it's interesting to consider how this situation might arise. Here's one example:

/** @public */
export declare class MyClass extends (class { }) { 
}

We can imagine other edge cases that should have references, but DeclarationReferenceGenerator hasn't implemented support (maybe an augmented type or somesuch).

Is "Encountered unexpected excerpt tokens" the best way to describe these cases? Here's a couple alternative ideas:

• Safe approach: "A canonical reference was not found in the extends clause" accurately reports the implementation logic that failed
• Friendly approach: "The extends clause does not refer to type that can be analyzed" tries to provide a meaningful interpretation of the situation that will hopefully be correct

[zelliott]

I preferred your "safe approach" to your "friendly approach" because I thought the latter was too generic (i.e. why can't the type be analyzed - is it because there was no reference, is it because there was no associated ApiModel, is it because declaration resolution failed, etc). See https://github.com/microsoft/rushstack/pull/3545/files#diff-f0516a1941ffe40f35eb325cb731d6d3af1663276d61824a4a6da636d91d278eR382-R383.

[octogonz]

This might arise if the ApiItem object was newly constructed but not added to an ApiModel yet. The problem isn't that we can't get the model, it's that no model has been associated. Maybe we could word it like:

"Unable to analyze references of API item ${next.displayName} because it is not associated with an ApiModel."

[zelliott]

Took this verbatim, see https://github.com/microsoft/rushstack/pull/3545/files#diff-f0516a1941ffe40f35eb325cb731d6d3af1663276d61824a4a6da636d91d278eR392-R393.