octogonz
commented
2 years ago
- If unexported base classes never/rarely represent bad API design, then the
ae-forgotten-exportwarning should never be surfaced.
The ae-forgotten-export warning was introduced because forgotten exports were frequently causing bad API releases, and remembering is not easy.
Consider this hypothetical example (with these declarations buried throughout a big pile of source files):
interface ILinkManagerOptions { . . . }
class LinkManager {
public constructor(options: ILinkManagerOptions);
. . .
}
class Link {
public constructor(manager: LinkManager);
public get manager(): LinkManager;
. . .
}
/** @public */
export class Document {
private _createLink(): Link;
}The trouble starts when somebody decides to promote _createLink() to be a public API, without realizing that Link is not exported yet. Unit tests seem to work fine when you call Document.createLink().url, so the NPM package gets published. But then customers start complaining that usage is awkward because they can't write const link: Link = doc.createLink();. To fix this, we go back and add export to Link, but now we realize that the API review people never looked closely at this API because they thought it was not exported. 😆 And then after that gets straightened out, the same embarrassment+correction plays out for LinkManager, ILinkManagerOptions, etc as we unravel a chain of forgotten exports.
Above, I am conflating several things:
- a declaration being exported (i.e. marked as
export) - a declaration being exposed as an API contract that consumers are meant to use
- a declaration having an official name that API consumers can refer to it as
- an internal developer workflow for catching mistakes
Certainly we can consider decoupling these things.
zelliott
schmidt-sebastian
h-a-n-a
Summary
The purpose of this issue is to kick off a discussion for how API Extractor can provide better support for handling unexported base classes. Consider the following scenario:
In the case above,
UserInputBaseis an anonymous implementation detail ofButtonandCheckboxand purposefully not exported.Today, API Extractor will generate documentation pages for
ButtonandCheckboxbut not forUserInputBase, as the latter isn't exported. Additionally, API Extractor will not generate documentation for any inherited members fromUserInputBasewithin the documentation pages forButtonandCheckbox. Lastly, it will surface anae-forgotten-exportwarning forUserInputBase.My understanding is that API Extractor treats the pattern above as "bad API design", hence the
ae-forgotten-exportwarning and poor documentation support. While in some cases this may indeed be bad API design (e.g. if you know developers will want to write functions that operate onUserInputBaseobjects), I think the false positive rate is too high. There are valid reasons to not exportUserInputBase. For example, the API author might want the ability to refactor the inheritance chain of these classes without making a breaking change. The author might want to renameUserInputBase, or remove it completely and move the inherited members directly into the sub-classes. These changes are harder to make ifUserInputBaseis exported.For these reasons, I think we should reconsider the following for this case:
ae-forgotten-exportwarning should be surfaced.Note that all of the above applies to interface inheritance as well.
Details
ae-forgotten-exportwarningI think the behavior here should be determined by the false positive rate for the
ae-forgotten-exportwarning for this case:ae-forgotten-exportwarning should never be surfaced.In my opinion, I think 1 or 2 make the most sense.
Better documentation support
Here are a few ideas for how API Extractor could provide better documentation support for this case. I don't have a super strong preference yet, I'll need some more time to think through them.
1. Include inherited members within exported sub class's API doc model
In this approach, we continue to omit the unexported base class from the API doc model and API report, but include any inherited members within the sub class's API doc model. So for the following scenario:
The API doc model would look like this:
If
Bwas exported, then the API doc model would look like this:Open questions:
B" from the API doc model entirely, assumingBis truly just an implementation detail ofA? Or renameBto something like(Anonymous)to indicate that its name is an unimportant? Or append something like(unexported)afterB?AextendsBextendsCextendsD, but onlyAandCare exported. I thinkAneeds to include all inherited members fromB,C, andD(note that we need to include the inherited members fromCandDbecause there's nothing in the doc model indicating thatBextendsC). Alternatively, we could simply not support this case if we think it represents bad API design.Bis in another package? IfBis in another package, then it must be exported (in order for it to be extended byA), and thus it should be included in its package's API doc model. We shouldn't need to include any inherited members in this case.2. Include unexported base classes in the API doc model
In this approach, we simply include unexported base classes in the API doc model and API report. Thus, an exported class's entire inheritance chain is always included in the API doc model. So for the following scenario:
The API doc model would look like this:
Open questions:
Bis unexported?3. Add a configuration to include forgotten exports in the API doc model
This is a solution to the more general use case of wanting to include "forgotten exports" in the API doc model. This is also the approach described at https://github.com/microsoft/rushstack/issues/1235#issuecomment-1129451803. We could add a new
includeForgottenExportsconfig to API Extractor that would cause all forgotten exports to be by default included in the API doc model and API report. We could also use some kind of doc comment tag to include/exclude declarations from these files at a per-declaration level.I'm not sure whether there's enough of a need for documenting forgotten exports other than the class/interface inheritance case. The only other example that comes to mind is something like this (see https://github.com/microsoft/rushstack/issues/1235#issuecomment-863603967):
But I'm receptive to API Extractor's current stance of not spending too much time/effort on perfectly documenting arbitrary type algebra.
Open questions: