rushstack icon indicating copy to clipboard operation
rushstack copied to clipboard

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

Open wcauchois opened this issue 3 years ago • 13 comments

Summary

I'm using these tools to document a TypeScript library, and I noticed that when you don't export a type from your index.ts, that type does not get included in API Documenter's output, and it won't be hyperlinked in any documentation.

It apparently doesn't get included in the "doc model" file from running api-extractor run.

Is there any way to change this behavior so that types may be documented without being exported? As it stands, I'm considering introducing a preprocessing step to replace ^type with export type temporarily in the TypeScript files themselves.

I realize that it may be the philosophy of this library that types should be exported if they're going to be documented, but I would rather not export these types because I don't want them to be part of the contract for my library. Nonetheless it would be useful to have them hyperlinked and documented.

Repro steps

Create e.g.

index.ts:

export type Foo = {
  numberField: number
  bar: Bar
}

type Bar = {
  stringField: string
}

Expected result:

User is able to navigate to "Bar" in the API documenter output so that they may see its definition.

Actual result:

User cannot navigate to "Bar" and no documentation for it is generated.

Details

Standard questions

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

Question Answer
@microsoft/api-extractor version? 7.28.2
Operating system? Mac
API Extractor scenario? docs (.api.json)
Would you consider contributing a PR? Yes
TypeScript compiler version? 4.2.4
Node.js version (node -v)? v12.22.7

wcauchois avatar Jul 01 '22 19:07 wcauchois

The example you provided seems like a pretty straightforward case of API Extractor's ae-forgotten-export warning. You mention:

I realize that it may be the philosophy of this library that types should be exported if they're going to be documented, but I would rather not export these types because I don't want them to be part of the contract for my library.

But Bar is part of your API contract per the exported Foo type. If a user has something like this in their code:

const foo: Foo = {
  numberField: 1,
  bar,
};

const bar = {
  stringField: 'Hello',
};

If you change the shape of Bar, this might break the user's code. So it's not clear to me how not exporting Bar prevents it from being part of your API contract. It seems like it's really just preventing them from explicitly typing bar (which also seems unfortunate and frustrating to users). Given this, is there a particular reason you're not exporting Bar?

I'd also recommend taking a look at https://github.com/microsoft/rushstack/issues/3430 - which concerns a specific scenario where we concluded that the ae-forgotten-export warning didn't make sense and also contains a lengthy discussion about the warning in general.

zelliott avatar Jul 07 '22 14:07 zelliott

Thanks for the response @zelliott! I think you're missing two cases where exporting a type name can undesirably increase the contract surface area of a library.

  1. Type renames. In your example, if bar is annotated as bar: Bar, that prevents us from renaming Bar internally (without changing its structure).

  2. Structure changes. In reality our types are fairly nested. Consider for example:

interface Response {
  user: User
}

interface User {
  fullName: FullName
}

interface FullName {
  firstName: string
  lastName: string
}

if we were to change it to:

interface Response {
  user: User
}

interface User {
  fullName: {
    firstName: string
    lastName: string
  }
}

then if we had exported FullName this would be a breaking change, while if we hadn't, this would not be a breaking change.

wcauchois avatar Jul 07 '22 15:07 wcauchois

Thanks for sharing those two cases, that helps me better understand your motivation.

Regardless of whether or not this is "good API design" (@octogonz is always well-spoken in this area), there's been a lot of previous discussion regarding "getting around" the api-forgotten-export rule (see https://github.com/microsoft/rushstack/issues/1235 for some past proposals). Today, you can disable the ae-forgotten-export warning in your api-extractor.json, but this still doesn't cause these "forgotten exports" to appear in either the API report or API doc model.

It might be helpful to hear a bit more about how you'd expect to be able to configure things. For example:

  1. Would you want to disable the warning on a per-project or per-declaration basis?
  2. Would you want forgotten exports to be included in the API report and API doc model files on a per-project or per-declaration basis?
  3. Would you want it so that disabling the warning for a declaration automatically causes that declaration to be included in the API report and API doc model files?

zelliott avatar Jul 07 '22 19:07 zelliott

Hey Zack! To answer your questions 1/2, I am looking to disable this behavior on a per-project basis (per-declaration would work, but per-project is what would satisfy my use case).

Going back to my original rationale: here's an example of some generated documentation where QueryDatabaseParameters is exported but the Path and Body parameters types are not exported.

image

As a user viewing this documentation, there's no action I can take to see what e.g. QueryDatabasePathParameters is, except by going into the source code (or using IntelliSense).

Sorry if I'm repeating myself here, I'm just trying to make clear what my problem is from a use-case oriented perspective.

Thank you for hearing me out!

wcauchois avatar Jul 13 '22 18:07 wcauchois

If we did add a project-wide configuration to disable the ae-forgotten-export warning and allow forgotten exports to be included in the API report and API doc model files, are there any instances in your project where you'd want the opposite behavior?

That is, I imagine the ae-forgotten-export warning is probably still useful for you in some scenarios, so disabling it entirely might lead to future API design bugs. Consider the following scenario:

interface SomeClassConfig {}

export class SomeClass {
  constructor(config: SomeClassConfig) {}
}

In the scenario above, you might have forgotten to export SomeClassConfig. However, with ae-forgotten-export disabled for your entire project, you wouldn't be warned of this.

If we instead supported getting around the forgotten export rule on a per-declaration basis, then you could disable it for cases like the QueryDatabaseParameters type union, but keep it enabled for cases like the SomeClassConfig scenario. Or maybe you think the benefit of the ae-forgotten-export rule is small, and not worth the extra dev-cost needed to configure the warning on a per-declaration basis.


It's also useful to think through how a documentation site like that generated by api-documenter might surface documentation on QueryDatabasePathParameters and QueryDatabaseBodyParameters. Would the site embed their documentation within the documentation page for QueryDatabaseParameters? That might not be ideal, because then if you had multiple API items that referenced these types, their documentation would be duplicated across different pages. Maybe instead the documentation page for QueryDatabaseParameters would link out to dedicated documentation pages for QueryDatabasePathParameters and QueryDatabaseBodyParameters? If we go with that approach:

  1. How does the QueryDatabaseParameters page link to the documentation pages for QueryDatabasePathParameters and QueryDatabaseBodyParameters? Do we render the type signature with links?
  2. How do we communicate on the documentation pages for QueryDatabasePathParameters and QueryDatabaseBodyParameters that these types are unexported?

zelliott avatar Jul 21 '22 16:07 zelliott

If we did add a project-wide configuration to disable the ae-forgotten-export warning and allow forgotten exports to be included in the API report and API doc model files, are there any instances in your project where you'd want the opposite behavior?

I don't believe so! Given the philosophy we've taken with our library, I don't think that warning is very useful to us.

If we instead supported getting around the forgotten export rule on a per-declaration basis, then you could disable it for cases like the QueryDatabaseParameters type union, but keep it enabled for cases like the SomeClassConfig scenario. Or maybe you think the benefit of the ae-forgotten-export rule is small, and not worth the extra dev-cost needed to configure the warning on a per-declaration basis.

Yep -- that last sentence.

It's also useful to think through how a documentation site like that generated by api-documenter might surface documentation on QueryDatabasePathParameters and QueryDatabaseBodyParameters. Would the site embed their documentation within the documentation page for QueryDatabaseParameters? That might not be ideal, because then if you had multiple API items that referenced these types, their documentation would be duplicated across different pages. Maybe instead the documentation page for QueryDatabaseParameters would link out to dedicated documentation pages for QueryDatabasePathParameters and QueryDatabaseBodyParameters? If we go with that approach:

Ideally: would link out to dedicated documentation pages, just like if it had been exported.

  1. How does the QueryDatabaseParameters page link to the documentation pages for QueryDatabasePathParameters and QueryDatabaseBodyParameters? Do we render the type signature with links?

Type signature with links, exactly. Like this:

image
  1. How do we communicate on the documentation pages for QueryDatabasePathParameters and QueryDatabaseBodyParameters that these types are unexported?

Get rid of "export" in the signature (highlighted section to be removed:)

image

I appreciate this discussion :)

wcauchois avatar Jul 21 '22 18:07 wcauchois

One basic proposal is to add a new "includeForgottenExports": boolean setting to the api-extractor.json file. If false (default value), API Extractor works just as it does today. If true, then all forgotten exports are included in the API report and API doc model files.

In the future, we can still always add support for per-declaration ae-forgotten-export warning suppression or per-declaration control over whether something should be included in the API report and API doc model files.

Then, some work could be done in api-documenter to support some of the ideas in https://github.com/microsoft/rushstack/issues/3513#issuecomment-1191791900.

@octogonz, WDYT?

zelliott avatar Jul 25 '22 17:07 zelliott

A little background about API Extractor's design philosophy:

if we were to change it to:

interface Response {
  user: User
}

interface User {
  fullName: {
    firstName: string
    lastName: string
  }
}

then if we had exported FullName this would be a breaking change, while if we hadn't, this would not be a breaking change.

Avoiding such a "breaking change" makes life easier for the maintainer of the library, but perhaps at the expense of its consumers. A couple examples:

  1. Suppose a consumer of your API wants to do something like this:

    function formatFullName(fullName: FullName): string {
      return `${fullName.firstName} ${fullName.lastName}`;
    }
    

    Since FullName is an unexported type, it is effectively anonymous. There is no good way for the consumer to declare the type of fullName. A diligent user might take the time to file an issue about this problem, but the answer is going to be "Oooh whoops, we didn't think of that. We'll export it for you. Please wait a month until the next SDK release." Most users won't even bother to report the problem. They'll just find an ugly hack and move on. Either way, not a great experience.

  2. The type name often conveys important information for consumers of your API. For example, compare this:

    interface IDocument {
      recipient: { name: string | undefined; address: string; };
    }
    

    ...with this:

    interface IDocument {
      recipient: IEmailAddress;
    }
    

octogonz avatar Jul 25 '22 19:07 octogonz

One basic proposal is to add a new "includeForgottenExports": boolean setting to the api-extractor.json file. If false (default value), API Extractor works just as it does today. If true, then all forgotten exports are included in the API report and API doc model files.

The setting name should probably distinguish "unexported APIs" versus "forgotten exports." If the person is intentionally NOT exporting it, then nothing was forgotten.

Whether or not an API is exported, I can imagine different combinations of answers to these questions:

  • Should this unexported API appear in the .api.md report? The @preapproved use case.

  • Should this unexported API appear in the .api.json file and API reference website? For example, IRushConfigurationJson is a big web of interfaces that are referenced by a public API but are uninteresting to API consumers. Maybe we could invent a doc tag that flags them to be excluded from the .api.json file?

If these states are individually controllable for each API using TSDoc tags, then perhaps @wcauchois's feature is a way to specify it globally for all unexported APIs. Something like excludeUnexportedApisFromApiReport=true and excludeUnexportedApisFromDocModel=true. Whereas a more careful user might prefer to specify it individually using TSDoc tags.

octogonz avatar Jul 25 '22 19:07 octogonz

A couple other thoughts:

Declaration references

If unexported API items are documented, then the {@link} tag needs to be able to reference them. The old TSDoc declaration reference syntax doesn't support this; however the new syntax does. (Back in May @rbuckton did a big chunk of work to move forward with this new syntax, but I unfortunately haven't had time to review it yet. 😢)

Ambiguous IDs

Unexported APIs will encounter an instance of the "ae-ambiguous-ids" problem from #1308:

  • The .d.ts rollup is a single TypeScript module, so it needs unique names. For example, it's possible for Thing from src/internal/Thing.ts to be unexported, while at the same time Thing from src/public/Thing.ts is exported. In this situation. the .d.ts rollup will rename src/internal/Thing.ts's declaration to Thing_1

  • The API reference website needs separate names, otherwise it's highly confusing for users

  • The API reference website also needs distinct URLs, e.g. example-api.com/pages/api/thing-1 versus example-api.com/pages/api/thing

  • These names/URLs must be stable. Suppose a third duplicate was introduced src/pathological/Thing.ts. It would be unacceptable for the existing example-api.com/pages/api/thing-1 to get shuffled to example-api.com/pages/api/thing-2 because a new API was introduced.

octogonz avatar Jul 25 '22 19:07 octogonz

The setting name should probably distinguish "unexported APIs" versus "forgotten exports." If the person is intentionally NOT exporting it, then nothing was forgotten.

It sounds like you prefer something like includeUnexportedApis to includeForgottenExports. I think I still prefer the latter for two reasons:

  1. In your IDocument example above, if someone purposefully doesn't export IEmailAddress, you're right that this item isn't "forgotten" from their point of view. However, it is still "forgotten" from API Extractor's point of view. I'm not sure that API Extractor cares about intention when deciding whether something is a forgotten export. Thus, I don't think the name includeForgottenExports is necessarily inaccurate.
  2. I think it'll be clearer to users what includeForgottenExports does than includeUnexportedApis. If I saw includeForgottenExports, I'd notice an immediate parallel to the ae-forgotten-exports warning (i.e. "Oh, this adds all of those ae-forgotten-exports declarations to my API report and API doc model!"). Whereas it wouldn't be immediately clear to me what an "unexported API" was.

If these states are individually controllable for each API using TSDoc tags, then perhaps @wcauchois's feature is a way to specify it globally for all unexported APIs. Something like excludeUnexportedApisFromApiReport=true and excludeUnexportedApisFromDocModel=true. Whereas a more careful user might prefer to specify it individually using TSDoc tags.

I like this plan. I think we should first add a global setting to toggle it on a project-level, and then add per-declaration support as a follow-up.

However, is there a situation where we'd want an unexported API to be included in the API report but not the API doc model (or vice versa)? It's certainly easier to control inclusion to both files with a single setting, so I'd prefer to do that if possible.

Lastly, wouldn't we prefer include...=false to exclude...=true so that the flags can default to false?

If unexported API items are documented, then the {@link} tag needs to be able to reference them

Are you sure? I know that unexported items are prefixed with ~. I can test this on my own but I think this might already work.

Ambiguous IDs

This problem already seems solved for .d.ts rollups, as you stated (renaming items to Thing_<index>).

I agree that in a perfect world these names & URLs would be stable. I thought about this for a bit, and I couldn't come up with a way to good automatically derive a stable name from the file/item itself. What if instead we add support for a TSDoc @alias tag that allows developers to rename an item for the documentation site (e.g. https://jsdoc.app/tags-alias.html). Then onus is on the developer to come up with a unique, stable name, and API Extractor can warn or something if it encounters duplicate aliases.

I will say that this is an existing bug with api-documenter today so I'm not sure that it should block this feature. For example, notice how at https://api.rushstack.io/pages/api-extractor-model.apiinitializermixin/, merged declarations all have the same URL, and thus certain API item pages don't even appear. Thus, I think this should be potentially tackled as a follow up task.

zelliott avatar Jul 25 '22 21:07 zelliott

However, is there a situation where we'd want an unexported API to be included in the API report but not the API doc model (or vice versa)?

No sure how realistic these are, but here's some ideas:

  • API report but not doc model: Suppose we want to know if this API changes, because it would imply a MAJOR SemVer bump for the library. But for some reason we don't want it to appear on the website, for example maybe the interfaces describe another company's copyrighted API interface. Or maybe for some technical reason the build tools copy the interfaces from another project that already has its own documentation.
  • Doc model but not API report: The documentation website is for an internal audience, who finds it convenient to include every single interface on the website. However the API review team doesn't care about some of these. (A classic example is telemetry data structures, which get endless adjustments by people who are monitoring service health, and nobody wants to review it.)

Lastly, wouldn't we prefer include...=false to exclude...=true so that the flags can default to false?

Yes. 👍 Those setting names were meant to be a rough sketch, not an actual proposal. :-)

Are you sure? I know that unexported items are prefixed with ~. I can test this on my own but I think this might already work.

The canonicalReference field in the .api.json file uses the new declaration reference syntax, which probably supports ~. The {@link} tag uses the old declaration reference syntax.

I agree that in a perfect world these names & URLs would be stable. I thought about this for a bit, and I couldn't come up with a way to good automatically derive a stable name from the file/item itself. What if instead we add support for a TSDoc @alias tag that allows developers to rename an item for the documentation site (e.g. https://jsdoc.app/tags-alias.html). Then onus is on the developer to come up with a unique, stable name, and API Extractor can warn or something if it encounters duplicate aliases.

I will say that this is an existing bug with api-documenter today so I'm not sure that it should block this feature. For example, notice how at https://api.rushstack.io/pages/api-extractor-model.apiinitializermixin/, merged declarations all have the same URL, and thus certain API item pages don't even appear. Thus, I think this should be potentially tackled as a follow up task.

We planned to solve the stability problem using the TSDoc @label tag. Some details appear in https://github.com/microsoft/rushstack/issues/881#issuecomment-461620185

I think we know how to solve most of the ae-ambiguous-ids GitHub issues. The challenge was just that it's difficult to keep straight all the different cases and their interrelationships (e.g. the URL should loosely correspond to the page title which should loosely correspond to the API name, but the forbidden characters and escaping mechanisms are different for each one). And the consequences of these bugs are somewhat rare, which did not motivate the potential work involved to fix them all.

I didn't mean to imply that #3513 should be blocked behind this problem, but it would be good to avoid digging the ae-ambiguous-ids hole deeper if possible. :-)

octogonz avatar Jul 25 '22 22:07 octogonz

No sure how realistic these are, but here's some ideas

Thanks for the ideas. I'm okay adding two flags, but we should just know that this'll complicate the design of any per-declaration TSDoc inclusion/exclusion tag a bit.

I'll take a stab at opening a PR for this today.

zelliott avatar Jul 25 '22 22:07 zelliott