microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.88k stars 595 forks source link

[api-documenter] Show a class/interface declaration's inherited members #3429

Closed zelliott closed 2 years ago

zelliott commented 2 years ago

Summary

Suppose we have the following class setup:

export class B {
  b: string;
}

export class A extends B {
  a: string;
}

Today, api-documenter will write two API documentation pages: one for A and one for B. It will list a under A and b under B. Notably, in the documentation page for A, there will be no indication that A inherits b (or any other additional properties/methods) other than the A extends B clause written to A's documentation page. A developer needs to "jump back and forth" between the documentation pages for A and B in order to understand A's full API.

I think we can do better. It should be possible to configure api-documenter to show a class's inherited members within its own API documentation page. The example above is for class inheritance, but all of the above applies to interface inheritance as well.

For a live example of this feature in TypeDoc, please see https://typedoc.org/api/classes/Application.html, in particular the "Inherited" checkbox on the top-right.

Details

Implementation details:

Some open questions:

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question Answer
@microsoft/api-documenter version? Latest
Operating system? All
Documentation target? All
Would you consider contributing a PR? Yes
TypeScript compiler version? N/A
Node.js version (node -v)? N/A
octogonz commented 2 years ago

(slightly off topic)

Notably, in the documentation page for A, there will be no indication that A inherits b (or any other additional properties/methods) other than the A extends B clause written to A's documentation page.

We do provide hyperlinks to the base classes.

Actually I just noticed that these links got broken during the Docusaurus migration:

https://api.rushstack.io/pages/api-extractor-model.apidocumenteditem/

image

@elliot-nelson is the api-documenter-docusaurus-plugin project where we should fix it?

octogonz commented 2 years ago
  • There should be an indication of some kind that a member is inherited (e.g. special formatting, a badge/tag, separate section, etc). What should this look like?

Maybe we could copy the model of docs.microsoft.com:

https://docs.microsoft.com/en-us/dotnet/api/system.windows.controls.primitives.buttonbase?view=windowsdesktop-6.0

image

Since the Markdown target doesn't provide an easy way to do an interactive [ ] Inherited checkbox, maybe this feature can be controlled via setting in api-documenter.json

octogonz commented 2 years ago
  • Should we also show members inherited from a declaration in a separate package? Suppose in the example above, B is in a separate package. The .api.json file for A's package won't include any information about B or b. If we wanted to support this, we'd need to make a change to api-extractor as well.

API Documenter already reads multiple .api.json files into a single model that supports cross-package inheritance.

I think the interesting design question is what to do when b.api.json is missing for some reason.

Two possibilities:

  1. API Documenter could visually indicate somehow that we know that there are probably other members being inherited, but we don't know what they are.
  2. When API Extractor is writing a.api.json it could serialize the signatures from B in that file, for usage in case b.api.json is not available. We can't assume that package b is well-behaved or even using TSDoc syntax, so I think we'd store minimal names and types with no additional detail. Note that this data may be inaccurate; for example, if a/package.json depends on "b": "^1.2.3" then in practice the actual signatures can differ depending on which package version was chosen by npm install. If c.api.json and a.api.json both have a copy of the same signatures from b, their copies might be inconsistent.

We could implement 1 first and introduce 2 later down the road if there is demand.

zelliott commented 2 years ago

Maybe we could copy the model of docs.microsoft.com

That sounds good to me.

Since the Markdown target doesn't provide an easy way to do an interactive [ ] Inherited checkbox, maybe this feature can be controlled via setting in api-documenter.json

That also sounds good to me. Maybe showInheritedMembers, which would default to false? By the way, can the api-documenter.json config file be documented next to https://api-extractor.com/pages/configs/api-extractor_json/, or is it still in some kind of "beta"? I don't believe there's any kind of documentation for this config file on the API Extractor website.

API Documenter already reads multiple .api.json files into a single model that supports cross-package inheritance.

I think the interesting design question is what to do when b.api.json is missing for some reason.

Yes - this is the case I was considering in my original issue comment, sorry for not clarifying. I'm onboard with doing 1 first and 2 down the road if there's demand.

zelliott commented 2 years ago

Another question: what if B is unexported (from the example above)? I'm aware that this is currently an ae-forgotten-export scenario. We could do the same thing as mentioned above for now: "visually indicate somehow that we know that there are probably other members being inherited, but we don't know what they are". Down the road, I think we should provide better support for this scenario (namely because I'm still somewhat skeptical of the ae-forgotten-export warning applying to unexported base classes in the first place), but I'll file a separate issue. :)

zelliott commented 2 years ago

Tricky inheritance situations:

// Generics
interface A<T> {
    a: T;
}
interface B<T = number> {
  b: T;
}
interface C extends A<number>, B {
  c: boolean;
}

// Type algebra madness
interface D {
  d: number;
  d2: boolean;
}
type E = Omit<D & {
  e: string;
}, 'd2'>;
interface F extends E {
  f: number;
}

These seem rather tough to resolve? I think we'll need to cut some scope from this feature. For now, we could:

zelliott commented 2 years ago

Same with resolving inherited members from mixins. Example from api-extractor-model:

declare const ApiClass_base: typeof ApiDeclaredItem & (new (...args: any[]) => ApiReleaseTagMixin) & (new (...args: any[]) => ApiTypeParameterListMixin) & (new (...args: any[]) => ApiNameMixin) & (new (...args: any[]) => ApiItemContainerMixin);
export declare class ApiClass extends ApiClass_base {

Interestingly, ApiClass_base is a "forgotten export" anyway:

zelliott commented 2 years ago

Okay! Resolving inherited members turns out to be in some ways easier and in some ways trickier than I originally thought. Here are different scenarios of class inheritance along with some notes on each scenario:

Scenario 1

export class A {}
export class B extends A {}
export class C extends B {}

This is trivial inheritance that should be fairly easy to implement in api-extractor-model. We'll definitely want the logic to be in api-extractor-model instead of api-documenter as even though this scenario's logic is fairly straightforward, it's still a non-trivial amount of code, and some of the subsequent scenarios quickly get complicated. Current design thoughts are as follows:

Scenario 2

export class A<T> {
  a: T;
}
export class B<T> extends A<T> {}

This case is also easy to support with no additional work to the above. Despite the presence of generics, the types of inherited methods are easy to determine.

Scenario 3

export class A<S> {
  a: S;
}
export class B<T> extends A<T> {}

The inherited member is a: S, but it may not be clear to a documentation reader what S is referring to. Ultimately, you'd want to surface this under the documentation section for B as a: T, but that requires parsing the type parameters within extends clauses, which is something we don't currently do.

Scenario 4

export class A<T = number> {
  a: T;
}
export class B extends A {}

The inherited member here is more accurately a: number, as opposed to a: T. This is also tricky to determine, as it requires parsing type parameters within extends clauses.

Scenario 5

export class A<T> {
  a: T;
}
export class B extends A<number> {}

Essentially the same as scenario 4.

Scenario 6

export class A<T = number> {}
export class B<T> {
    b: T;
}
export class C extends B<A> {}

This is a more convoluted inheritance where the inherited member is most accurately b: A<number>. Again, tricky to determine.

Scenario 7

export class B extends MyMixin(A) {}

My understanding here is that TypeScript will create a synthetic B_base entity in the type declaration file that will look something like this, depending on the exact implementation of the mixin:

declare const B_base: {
    new (...args: any[]): {};
} & typeof A;

Two notable things: (1) B extends a const, (2) that const is unexported. API Extractor doesn't have support right now for 1 and addressing 2 would require either (1) exporting MyMixin(A) or (2) solving https://github.com/microsoft/rushstack/issues/3430.

Scenario 8

export const A: {
  new (...args: any[]): {};
} = ...
export class B extends A {}

Similar to scenario 7 except the const is exported.

Scenario 9

// `A` is not in the `ApiModel`, perhaps an external package like react.
export class B extends A {}

We don't know what the inherited members are here, so the current plan is to surface some kind of message on the docs site saying this.

Scenario 10

export class B extends (class {}) {}

This turns out to be similar to scenario 7. The anonymous base class is pulled out into a const and unexported.


Ultimately, there are a few different categories of complications:

I'll update this issue if I find more interesting class inheritance scenarios.

zelliott commented 2 years ago

Update: So I managed to update API Extractor to extract excerpt tokens for not only the extendsType but also any type arguments within the extendsType. So in a scenario such as scenario 5 above (reproduced below):

export class A<T> {
  a: T;
}
export class B extends A<number> {}

When processing B, we now know that (1) B extends something of type A<number> and (2) the type argument of the thing B extends is number. The relevant section of the .api.json for B looks something like this:

"excerptTokens": [
  {
    "kind": "Content",
    "text": "export declare class B extends "
  },
  {
    "kind": "Reference",
    "text": "A",
    "canonicalReference": "api-documenter-test!A:class"
  },
  {
    "kind": "Content",
    "text": "<"
  },
  {
    "kind": "Content",
    "text": "number"
  },
  {
    "kind": "Content",
    "text": "> "
  }
],
"extendsTypes": [
  {
    "tokenRange": {
      "startIndex": 1,
      "endIndex": 5
    },
    "typeArgumentTokenRanges": [
      {
        "startIndex": 3,
        "endIndex": 4
      }
    ]
  }
],

This helps but ultimately isn't a complete solution. We'd need to use this data to determine that the inherited member a: T should really be a: number when documented under B. This requires knowing about the existence of the T within the ApiProperty for a, which we currently don't know.

But at this point, I'm thinking this is somewhat of a round-about way of determining that the inherited member a should be documented as a: number under B. Maybe we should just be using the type checker instead to find B's inherited properties. But this would potentially require storing inherited properties in the .api.json file.

EDIT: I did some quick testing, and it seems pretty straightforward to use the type checker to solve essentially all of the scenarios in https://github.com/microsoft/rushstack/issues/3429#issuecomment-1137990956. But again, if we're using the type checker to find inherited members, I think we'd need to add them to the .api.json file.

zelliott commented 2 years ago

@octogonz and I discussed the scenarios 3-6 today (i.e. inheritance involving type arguments). We agreed that, ideally, the inherited API item would be surfaced with the resolved "contextual" type. However, this is rather non-trivial to implement (e.g. involves changes to api-extractor, non-trivial type checker usage, storing additional inherited members in the .api.json file) and showing the "incorrect" type (i.e. the type as-written in the base class) isn't that confusing as there's already a note next to the item indicating that it's inherited.

Thus, we decided to just show the type as-written in the base class instead of showing the resolved contextual type. We can reconsider showing the resolved contextual type if there's demand from the community in the future.

So where does this leave things? Repeating the list of complicated scenario categories mentioned in https://github.com/microsoft/rushstack/issues/3429#issuecomment-1137990956: