-
Notifications
You must be signed in to change notification settings - Fork 657
[api-documenter] Generate DocFX UIDs using new DeclarationReference algorithm #1450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| // 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. |
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it does not. We would have to submit a PR to dotnet/docfx for this change.
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've mentioned this before, but I have a workaround I use for https://github.com/esfx/esfx that involves changes to support namespaces properly:
- yamlDocumenter.js
- UniversalReference.common.js
- namespace.tmpl.partial (NOTE: I redesigned the template organization to make it easier to tinker with)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I planned to do so eventually, but not until my schedule has settled down.
build-tests/api-documenter-test/etc/yaml/api-documenter-test/docbaseclass.yml
Show resolved
Hide resolved
|
@octogonz I assume that in order to verify the |
@Vitalius1 Strictly speaking, no. The .api.json file format is backwards compatible. The But as far as test validation, the first priority is to validate that all the latest stuff works together. Testing compatibility with older .api.json files is a secondary priority. |
| nameParts.unshift(current.displayName); | ||
| } | ||
|
|
||
| return nameParts.join('.'); |
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.
If the DeclarationReference already is namespace qualified, could you leverage that? I'm thinking of adding some additional utility methods/properties to the DeclarationReference API to make that even easier.
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.
This is not really a DeclarationReference. It's a title that is shown in the document, in a specific context.
I'm thinking of adding some additional utility methods/properties to the
DeclarationReferenceAPI to make that even easier.
@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 DeclarationReference API. We can follow up about this separately.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought had been to add a few members to DeclRef and related objects:
isFullyQualified-trueif the DeclRef has no missing components (:function,:constructor,:index,:call,:new, and:complexwould also require an overload index), otherwisefalseisReachable-trueif the DeclRef contains no Local (~) navigations, otherwisefalse.isStatic-trueif the DeclRef contains only Static (.) navigations, otherwisefalse.isInstance-trueif the DeclRef contains any Instance (#) navigations, otherwisefalse.namespaceQualifiedName- Returns the string value of the DeclRef's symbol excluding any meaning or overload index:my-package:a.b.c:member->a.b.cmy-package:a.b.c:class->a.b.cmy-package:a.b.c:function(1)->a.b.c- NOTE: I considered replacing
#with.prototype.for this as well...
namespaceName- Returns the string value of the DeclRef's symbol excluding the outermost ComponentPaths that do not represent a namespace member, meaning, or overload index (i.e.my-package:a.b.c:member->a.b).- NOTE: This would have been a good use case for
enummember, as anenummemberis kind ofmember-like and kind of avar-like export of its containing namespace... my-package:a.b.c:member->amy-package:a.b.c:class->a.bmy-package:a.b.c:var->a.bmy-package:a.b.c:namespace->a.b
- NOTE: This would have been a good use case for
classQualifiedName- Similar tonamespaceQualifiedName, but only includes the containing class name for members:my-package:a.b.c:member->b.cmy-package:a.b#c:member->b#cmy-package:a.b.c:var->my-package:a.b:constructor->b
className- Gets the name of the containing class/interface/enum:my-package:a.b.c:member->bmy-package:a.b#c:member->bmy-package:a.b.c:var->my-package:a.b:constructor->b
name- Gets the name of the symbol:my-package:a.b.c:member->cmy-package:a.b.c:class->c
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);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.
my-package:a.b.c:member->b.cmy-package:a.b#c:member->b#c
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 DeclarationReference API should try to solve a problem that is really about documentation formatting. For example, although # and (2) are important operators in the {@link} notation, I'm not sure they will look nice on the documentation website. Utilities.getConciseSignature() returns (arg1,arg2) instead of the overload index because it's a better documentation experience, despite being a worse design for the UID requirements.
If we have my-package!myNamespace.MyClass#staticMember(2), the kinds of strings we'd want from a doc formatting API might be more like this:
myNamespace (namespace)myNamespace.MyClass (class)MyClass (class)staticMember(arg1,arg2)MyClass.staticMember(arg1,arg2) (static)
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.
| - 'api-documenter-test!DocClass1#deprecatedExample:member(1)' | ||
| - 'api-documenter-test!DocClass1#exampleFunction:member(1)' | ||
| - 'api-documenter-test!DocClass1#exampleFunction:member(2)' | ||
| - 'api-documenter-test!DocClass1#interestingEdgeCases:member(1)' |
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.
Even though they can't collide, looking at this kind of makes me want to have :property, :accessor, :method, and :enummember, but this is probably fine.
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.
We want this to be the same notation that humans use when creating {@link} tags, though. Inexperienced developers may have trouble understanding the distinction between an :accessor versus a :property, and they may wonder why an :enummember is treated specially.
Most of the selector names are easy to learn because they correspond to the TypeScript keyword that introduces them (e.g. constructor, var, function, etc). Thus people can learn that :member is the catch-all for constructs that don't have a natural TypeScript keyword. (The :index and :call selectors are the only exceptions to this rule, but they are uncommon and somewhat special as they don't have an identifier.)
| // } | ||
| // } | ||
| // | ||
| // In the above example, "member(x, y)" does not appear as "N1.N2.C.member(x,y)" because YamlDocumenter |
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 of x with 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
|
@octogonz, Everything looks in order. This does fix #1052, which is excellent. I'm excited to see #1337 unblocked! |
|
@octogonz I finally got it tested and the issue mentioned in #1252 is gone. Although there is something broken in the logic of filtering the TOC items within the |
…-uids [api-documenter] Generate DocFX UIDs using new DeclarationReference algorithm
Update api-documenter to generate DocFX UIDs using use @rbuckton's new DeclarationReference syntax.
Fixes #1252
Fixes #1052
@Vitalius1 @AlexJerabek @natalieethell Could you test this on your DocFX projects and make sure that (1) nothing is broken and (2) the above issues are fixed?
(I will tackle the filename/URL naming in a subsequent PR, because I'm realizing the solution for that problem is slightly different.)
Huge thanks to @rbuckton for designing the new UIDs! 😊🎉