Skip to content

Conversation

@octogonz
Copy link
Collaborator

@octogonz octogonz commented Aug 8, 2019

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! 😊🎉

// 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.
Copy link
Collaborator Author

@octogonz octogonz Aug 8, 2019

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

Copy link

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.

Copy link

@rbuckton rbuckton Aug 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

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.

Copy link

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.

@Vitalius1
Copy link
Contributor

@octogonz I assume that in order to verify the api-documenter I would need the new api.json files outputted by api-extractor?

@octogonz
Copy link
Collaborator Author

octogonz commented Aug 8, 2019

I assume that in order to verify the api-documenter I would need the new api.json files outputted by api-extractor?

@Vitalius1 Strictly speaking, no. The .api.json file format is backwards compatible. The canonicalReference JSON field is for informational purposes only, and will get regenerated while loading the file.

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('.');
Copy link

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.

Copy link
Collaborator Author

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 DeclarationReference API 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.

Copy link

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.

Copy link

@rbuckton rbuckton Aug 8, 2019

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 - true if the DeclRef has no missing components (:function, :constructor, :index, :call, :new, and :complex would also require an overload index), otherwise false
  • isReachable - true if the DeclRef contains no Local (~) navigations, otherwise false.
  • isStatic - true if the DeclRef contains only Static (.) navigations, otherwise false.
  • isInstance - true if the DeclRef contains any Instance (#) navigations, otherwise false.
  • namespaceQualifiedName - Returns the string value of the DeclRef's symbol excluding any meaning or overload index:
    • my-package:a.b.c:member -> a.b.c
    • my-package:a.b.c:class -> a.b.c
    • my-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 an enummember is kind of member-like and kind of a var-like export of its containing namespace...
    • my-package:a.b.c:member -> a
    • my-package:a.b.c:class -> a.b
    • my-package:a.b.c:var -> a.b
    • my-package:a.b.c:namespace -> a.b
  • classQualifiedName - Similar to namespaceQualifiedName, but only includes the containing class name for members:
    • my-package:a.b.c:member -> b.c
    • my-package:a.b#c:member -> b#c
    • my-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 -> b
    • my-package:a.b#c:member -> b
    • my-package:a.b.c:var ->
    • my-package:a.b:constructor -> b
  • name - Gets the name of the symbol:
    • my-package:a.b.c:member -> c
    • my-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);

Copy link
Collaborator Author

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.c
  • my-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)'
Copy link

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.

Copy link
Collaborator Author

@octogonz octogonz Aug 8, 2019

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
Copy link

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.

Copy link
Collaborator Author

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.

Copy link

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

@AlexJerabek
Copy link
Contributor

@octogonz,
I tested this out against the Office JS reference docs. OfficeDev/office-js-docs-reference#477

Everything looks in order. This does fix #1052, which is excellent. I'm excited to see #1337 unblocked!

@Vitalius1
Copy link
Contributor

@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 ExperimentalYamlDocumenter now. I say have this change merged and the experimental logic can be adjusted separately when I have all other parts working.

@octogonz octogonz merged commit 1f18a53 into master Aug 12, 2019
@octogonz octogonz deleted the octogonz/ad-new-yaml-uids branch August 12, 2019 21:09
rikhoffbauer pushed a commit to rikhoffbauer/microsoft__api-documenter that referenced this pull request Mar 23, 2020
…-uids

[api-documenter] Generate DocFX UIDs using new DeclarationReference algorithm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants