[api-documenter] Generate DocFX UIDs using new DeclarationReference algorithm #1450
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,6 @@ import { | |
| ApiEnumMember, | ||
| ApiClass, | ||
| ApiInterface, | ||
| ApiMethod, | ||
| ApiMethodSignature, | ||
| ApiConstructor, | ||
|
|
@@ -551,36 +550,10 @@ export class YamlDocumenter { | |
|
|
||
| /** | ||
| * Calculate the DocFX "uid" for the ApiItem | ||
| * Example: `node-core-library!JsonFile#load` | ||
| */ | ||
| protected _getUid(apiItem: ApiItem): string { | ||
| return apiItem.canonicalReference.toString(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -677,11 +650,46 @@ export class YamlDocumenter { | |
|
|
||
| private _getYamlItemName(apiItem: ApiItem): string { | ||
| if (apiItem.parent && apiItem.parent.kind === ApiItemKind.Namespace) { | ||
| // If the immediate parent is a namespace, then add the namespaces to the name. For example: | ||
| // | ||
| // // Name: "N1" | ||
| // export namespace N1 { | ||
| // // Name: "N1.N2" | ||
| // export namespace N2 { | ||
| // // Name: "N1.N2.f(x,y)" | ||
| // export function f(x: string, y: string): string { | ||
| // return x + y; | ||
| // } | ||
| // | ||
| // | ||
| // // Name: "N1.N2.C" | ||
| // export class C { | ||
| // // Name: "member(x,y)" <=========== | ||
| // public member(x: string, y: string): string { | ||
| // return x + y; | ||
| // } | ||
| // } | ||
| // } | ||
| // } | ||
| // | ||
| // In the above example, "member(x, y)" does not appear as "N1.N2.C.member(x,y)" because YamlDocumenter | ||
| // embeds this entry in the web page for "N1.N2.C", so the container is obvious. Whereas "N1.N2.f(x,y)" | ||
| // needs to be qualified because the DocFX template doesn't make pages for namespaces. Instead, they get | ||
| // flattened into the package's page. | ||
|
Collaborator
Author
There was a problem hiding this comment. @rbuckton Does the DocFX template have support for namespaces yet? If so, then we could eliminate this awkward workaround and generate proper pages for them. For an example of how this is currently getting rendered, take a look at: https://docs.microsoft.com/en-us/javascript/api/office?view=excel-js-preview#office-initialize There was a problem hiding this comment. No, it does not. We would have to submit a PR to dotnet/docfx for this change. There was a problem hiding this comment. I've mentioned this before, but I have a workaround I use for https://github.com/esfx/esfx that involves changes to support namespaces properly:
Example: https://esfx.js.org/esfx/api/equatable/comparable_namespace.html#equatable_Comparable_Namespace
Collaborator
Author
There was a problem hiding this comment. Have you considered opening a PR for DocFX itself? I think everyone who uses TypeScript namespaces would be greatly appreciative of this change. There was a problem hiding this comment. I planned to do so eventually, but not until my schedule has settled down. |
||
| const nameParts: string[] = [ Utilities.getConciseSignature(apiItem) ]; | ||
|
|
||
| for (let current: ApiItem | undefined = apiItem.parent; current; current = current.parent) { | ||
| if (current.kind !== ApiItemKind.Namespace) { | ||
| break; | ||
| } | ||
|
|
||
| nameParts.unshift(current.displayName); | ||
| } | ||
|
|
||
| return nameParts.join('.'); | ||
|
There was a problem hiding this comment. If the
Collaborator
Author
There was a problem hiding this comment. This is not really a
@rbuckton In order to integrate this into the TSDoc parser, we will need to integrate it into NodeParser, and the objects need to replace the earlier DocDeclarationReference AST node type. Ideally we would do that in a way that eliminates the need for a standalone There was a problem hiding this comment. No worries, was just thinking about whether the DeclarationReference could be reused for this because it already supports the dot-qualified formatting, but I wouldn't worry about changing anything here. There was a problem hiding this comment. My thought had been to add a few members to DeclRef and related objects:
Then you could have done something like: const namespaceName = apiItem.canonicalReference.namespaceName
return (namespaceName ? namespaceName + '.' : '') + Utilities.getConciseSignature(apiItem);
// or, if `Utilities.getConciseSignature` is updated to return only the argument list...
return apiItem.canonicalReference.namespaceQualifiedName + Utilities.getConciseArgumentList(apiItem);
Collaborator
Author
There was a problem hiding this comment.
It might be nice to have a standardized API for rendering names, especially if they may contain problematic characters like ECMAScript symbols, quoted strings, or overloads. However, I'm unsure whether the If we have
The api-extractor-model API might be a better place to implement this, since it has access to information like whether the parent is a namespace or not, and what are the argument names. |
||
| } else { | ||
| return Utilities.getConciseSignature(apiItem); | ||
| } | ||
| } | ||
|
|
||
| private _getYamlFilePath(apiItem: ApiItem): string { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any concern with TOC collisions with
class C { static x; x; }? Both would have the TOC display name ofxwith no disambiguation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does DocFX create TOC entries for class members? I didn't think it did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure at the moment, but you may have to disambiguate other items in the TOC, such as merging a function or a variable and an interface with the same name. I had to workaround this for esfx: https://github.com/esfx/esfx/blob/bbc36650efa6a88c9d998b0dafb53e8421513898/scripts/docs/yamlDocumenter.js#L214-L216