[api-extractor] Deeply linked types

[rbuckton]

From #1317 (comment). This adds support for deeply linked types when emitting to Yaml for DocFX. It does this by utilizing DocFX's support for language-specific type specifications in references.

[AlexJerabek]

Thanks for putting this PR together @rbuckton. I tested this on the office-js doc set and received the following error:

Error: JSON validation failed:
yaml\excel\excel.bindingcollection.yml
Error: #/references
       Array items are not unique (indexes 0 and 1)

I'd love to sync up and continue testing this to ensure it works with our doc set and fixes #867.

[rbuckton]

Can you reply with a copy of the relevant portion of the yaml file and I'll look into what is causing the duplication?

[AlexJerabek]

Here's a link to the JSON file that caused the error.

Here's a gist with the JSON for the Excel.BindingCollection mentioned in the above error.

Please let me know if you need anything else.

[rbuckton]

The yaml\excel\excel.bindingcollection.yml file mentioned in the error would be more useful for diagnosing the issue

Disregard: I was able repro this with the linked json file.

[rbuckton]

@AlexJerabek should be addressed by e9d3df9

[AlexJerabek]

@rbuckton Tested the latest version on this staging branch. Looks good. Thank you.

The only problem is the changes break our custom example snippet mapping in OfficeYamlDocumenter. The console.error(colors.yellow('Warning: Unused snippet ' + apiName)); is being hit for every snippet. That is probably on us to investigate and fix, but if you see an obvious naming change, please let us know.

[AlexJerabek]

Looks like the issues in OfficeYamlDocumenter are because of the improved UIDs. That's on us to fix our mapping files. Thanks for addressing #867.

[octogonz]

@rbuckton following up from Gitter:

My main feedback was that the UID class seems out of place in api-extractor-model and as a field in the .api.json file format, because the Universal ID concept is specific to DocFX. This new notation solves the same problems as TSDoc "declaration references" (used in {@link} tags for example). Your notation improves on the existing syntax by (1) increasing compatibility with JSDoc namepaths and (2) simplifying the strings overall. Thus it makes sense to integrate this into TSDoc, rather than introducing a second different notation that is DocFX-specific.

Also, the code review may go quicker if the UID parser/emitter is a separate PR from the API Documenter updates (even though it's certainly helpful to see them together in e9d3df9).

That said, you have fixed an important problem for YamlDocumenter that I'm sure @AlexJerabek is eager to use, so we should be careful not to delay things unnecessarily. Thus I'd like to suggest the following:

1. Rename Uid.ts to DeclarationReference.ts
2. Open a separate PR to add this new DeclarationReference.ts class under a tsdoc/src/beta in the TSDoc project, which we can designate as a "proposed syntax change" not ready for general usage yet
3. Then update your PR here to import the "beta" class from @microsoft/tsdoc package.
4. Rename .api.json field from "uid" to "declarationReference" (or some other name that is not DocFX-specific). This can completely replace "canonicalReference". That field was halfbaked and not really used by the deserializer or API Documenter, so I wouldn't consider it a breaking API change to remove.

After both PRs are merged, the follow up steps would be:

• Create a PR updating DeclarationReferences.ts to show how it would look with your new notation, and solicit feedback from some interested people
• If everyone is cool with it, then we can modify NodeParser.ts to integrate your parser/emitter into the DocDeclarationReference API. This /will/ be a breaking change for both TSDoc and API Extractor, both API and input specification. We would major bump API Extractor to version 8 for that change.
• Incorporate your notation into a formal TSDoc spec document (which we've been planning to write up for a while; the ae-ambiguous-ids problem you're helping with here is one of a few remaining loose ends that I wanted to solve before writing up the spec.)

(I'm not asking you to do these follow up steps, since your goal in this PR was merely to improve the doc generation. We can handle this part. If you /do/ have time to help with that, it would be super awesome of course. :-) )

Let me know what you think.

[octogonz]

Also congrats on getting the 1337 integer value! 😎

[rbuckton]

Thus I'd like to suggest the following:

I will see if I can put together a PR for tsdoc next week.

[octogonz]

I will see if I can put together a PR for tsdoc next week.

@rbuckton Do you want some help with this? I might have some time over the holiday.

[AlexJerabek]

Hi @rbuckton. What's the status of this PR? We're eager to integrate these changes with our documentation tooling.

[octogonz]

@AlexJerabek We're workin' on it. I merged and published microsoft/tsdoc#172 today, so this PR should be unblocked.

[octogonz]

@rbuckton I'm trying to upgrade your PR branch to use the TSDoc version of the API, but I don't seem to have permissions to push to your branch. Could you fix that?

[octogonz]

In the test, this input:

export interface IDocInterface6 {
  . . .
  genericReferenceMethod<T>(x: T): T;
}

...gets emitted as this YAML:

  - uid: 'api-documenter-test!IDocInterface6#genericReferenceMethod:member(1)'
    name: genericReferenceMethod(x)
    fullName: genericReferenceMethod(x)
    langs:
      - typeScript
    type: method
    syntax:
      content: 'genericReferenceMethod<T>(x: T): T;'
      return:
        type:
          - T
        description: ''
      parameters:
        - id: x
          description: ''
          type:
            - T
      typeParameters:
        - id: T

...with a reference like this:

references:
  - uid: T

If we really want to treat T as a referencable thing, then it should get a unique UID notation (e.g. something like api-documenter-test!IDocInterface6#genericReferenceMethod:member(1).T). The _knownTypeParameters lookup here appears to be a workaround for the problem that the name T is reused for many different things, but is being treated as a UID (i.e. declaration reference) which is expected to be unique.

But stepping back for a moment, I'm not sure I see any value in linking to generic parameter declarations, for the same reason that we don't link to regular function parameters either. They are very local things, and their documentation tends to be very small, so it's quite sufficient to link to the associated function/method/class instead.

@rbuckton Thus, it seems like we could completely eliminate this _knownTypeParameters logic by not generating a canonicalReference for T in the first place.

[rbuckton]

The reason this is here is to prevent a possible collision with a type in scope that has the same name as a type parameter:

interface Foo {}
declare function f<Foo>(f: Foo): Foo;

In the output yaml, the Foo interface has uid: !Foo:interface and name: Foo. Unfortunately, I cannot recall whether docfx will attempt to resolve a type: Foo against a local name: if it can't resolve the uid, but I believe that was the reason why I added this in the first place. It's been more than a few weeks since I first looked at this.

[rbuckton]

I can remove this logic. We can decide if we want to add it back in if we see issues with docfx.

[octogonz]

If I remember right, DocFX's file format has a design flaw where if a type value matches a UID, then it is assumed to be a reference to that UID. They provide no escaping mechanism that could be used to represent a token that accidentally matches a UID, but is meant to be plain text (because it does not correspond to any linkable object). If that's true, it is a flaw in the DocFX file format.

However, luckily your "canonical reference" notation will always contain a ! character, which never appears in a valid type expression. Thus, API Extractor should never encounter such a conflict.

It was possible type parameters only because the generic parameter is appearing in the canonicalReference field:

{
  "kind": "Reference",
  "text": "T",
  "canonicalReference": "T"  // <=== not a correct reference
},

[octogonz]

BTW for an input like this:

export function exampleFunction(x: number): number {
  return x;
}

...it was an accident that identifiers like exampleFunction and x are being represented as ExcerptTokenKind.Reference. The initial implementation merely sought to demonstrate the Content/Reference distinction for consumers of the API, but without actually emitting any link targets. It simply assigned ExcerptTokenKind.Reference whenever span.kind === ts.SyntaxKind.Identifier, without considering the kind of identifier.

Instead of this:

          "excerptTokens": [
            {
              "kind": "Content",
              "text": "export declare function "
            },
            {
              "kind": "Reference",                // <===
              "text": "exampleFunction"
            },
            {
              "kind": "Content",
              "text": "("
            },
            {
              "kind": "Reference",                // <===
              "text": "x"
            },
            {
              "kind": "Content",
              "text": ": "
            },
            {
              "kind": "Content",
              "text": "number"
            },
            {
              "kind": "Content",
              "text": "): "
            },
            {
              "kind": "Content",
              "text": "number"
            },
            {
              "kind": "Content",
              "text": ";"
            }
          ],

...the "kind" should really be "Content" everywhere. We should only use "Reference" when there we have an actual "canonicalReference" link target.

(BTW if for some reason we needed to distinguish exampleFunction versus ): tokens, then I'd suggest to introduce a new kind such as ExcerptTokenKind.SimpleIdentifier for identifiers that lack a canonicalReference. We could also perhaps convert IExcerptToken to be a discriminated union to make this more clear.)

[rbuckton]

The problem I see is that the ExcerptBuilder doesn't know anything about the token in question. If we remove the canonicalReference for exampleFunction here, then we would have nothing to link to if we built an excerpt for the type typeof exampleFunction. It may be extraneous, but I don't think we should remove it.

[octogonz]

Take a look at PR rbuckton#2 (into your PR branch).

I changed this:

src/generators/ExcerptBuilder.ts

      if (span.kind === ts.SyntaxKind.Identifier) {
        const name: ts.Identifier = span.node as ts.Identifier;
        const canonicalReference: DeclarationReference | undefined = isDeclarationName(name)
          ? undefined
          : state.referenceGenerator.getDeclarationReferenceForIdentifier(name);
        ExcerptBuilder._appendToken(excerptTokens, ExcerptTokenKind.Reference,
          span.prefix, state, canonicalReference);
      } else {
        ExcerptBuilder._appendToken(excerptTokens, ExcerptTokenKind.Content,
          span.prefix, state);
      }

...to this:

      let canonicalReference: DeclarationReference | undefined = undefined;

      if (span.kind === ts.SyntaxKind.Identifier) {
        const name: ts.Identifier = span.node as ts.Identifier;
        if (!isDeclarationName(name)) {
          canonicalReference = state.referenceGenerator.getDeclarationReferenceForIdentifier(name);
        }
      }

      if (canonicalReference) {
        ExcerptBuilder._appendToken(excerptTokens, ExcerptTokenKind.Reference,
          span.prefix, state, canonicalReference);
      } else {
        ExcerptBuilder._appendToken(excerptTokens, ExcerptTokenKind.Content,
          span.prefix, state);
      }

The test-outputs/typeOf2 still correctly links typeof f:

            {
              "kind": "Content",
              "text": "typeof "
            },
            {
              "kind": "Reference",
              "text": "f",
              "canonicalReference": "api-extractor-scenarios!f:function"
            },

[octogonz]

No reference should be generated here, since the api-extractor-scenarios package does not export a symbol called Promise.

[rbuckton]

The problem is that it imports Promise from ./localFile as the alias MyPromise. I get the name Promise by resolving the alias.

Perhaps this should have been "api-extractor-scenarios/localFile!Promise:class"? The problem with that is that api-extractor doesn't currently seem to support anything other than package-level exports.

Also, you include the definition of MyPromise as an unexported class in the rollup.d.ts named Promise_2, so perhaps this should have been "api-extractor-scenarios!~Promise_2:class"?

[rbuckton]

Also, the ExcerptBuilder can't know whether the custom Promise here might be publicly exported as some completely different name by the package.

[octogonz]

@rbuckton In @adventure-yunfei's PR #1005 (an earlier attempt at this), the approach was to build a Map<ts.Symbol, ApiItem> for all exported APIs, and then consult that when generating the excerpt references. (He didn't consider how to choose among multiple exports, but a heuristic would be reasonable.)

Also, you include the definition of MyPromise as an unexported class in the rollup.d.ts named Promise_2, so perhaps this should have been "api-extractor-scenarios!~Promise_2:class"?

As far as I can see, the purpose of these references is for linking to documentation entries. As long as API Extractor does not generate documentation for unexported symbols, then there isn't much benefit in referring to them. The api-extractor-scenarios.api.json captures the complete inventory of possible documentation entries, we based on that we should be able to determine that api-extractor-scenarios!Promise:class isn't linkable, and thus should be omitted.

I'd like to get your PR merged though, so maybe we should postpone this issue and tackle it in a subsequent PR (unless it's a very simple addition).

[rbuckton]

The api-extractor-scenarios.api.json captures the complete inventory of possible documentation entries

I've mentioned before that this is an incorrect assumption. DocFX supports something called an xrefmap.yml file that lets you link to documentation not included in your project. This is useful if you are building an API on top of an external API and you want to link to the external API's documentation as well.

[octogonz]

To clarify, I was referring to the subset of links that refer to declarations defined by the current package. For example, the ts.SourceFile containing the (followed) declaration for MyPromise is part of the file set for the current package, thus we can be sure there is no documentation entry for it. There's no way it could appear in xrefmap.yml, right?

For references to symbols imported from other packages, we should preserve those in case they are documented as you say. BTW if an external package publishes a tsdoc-metadata.json file, then we could possibly prove that its declarations would not appear in xrefmap.yml also, I think.

Does that make sense?

[rbuckton]

To clarify, I was referring to the subset of links that refer to declarations defined by the current package.

What is the action item here then? Do I need to make a change in api-extractor to skip adding a canonicalReference to an excerpt for a non-exported member in the same package? Its unfortunate that those types will be printed in the excerpt but unreachable in the documentation. Should I be introducing a symbol -> ApiItem mapping as you mentioned to verify this, or some other mechanism?

BTW if an external package publishes a tsdoc-metadata.json file, then we could possibly prove that its declarations would not appear in xrefmap.yml also, I think.

I suppose it does, though I'm not sure I want to take on the added complexity of that in this PR.

[octogonz]

@rbuckton I think so, but let's postpone that for a later PR. I don't see any practical problems with the current implementation. As mentioned on Gitter, tomorrow we'll ask SharePoint to validate against their full data set, and if there are no issues then we should be good to finally merge it. (Hurray!)

BTW I'm really excited about this feature! Thanks so much for finishing it. After the PR is merged, I want to get it enabled for markdown output as well.

[octogonz]

BTW here's an interesting edge case to think about (although possibly we can tackle it later in a separate PR):

// Let's say this is how A is named in the local file where it is declared
class A {
  public static create(): A {
    return new A();
  }
}

// But when we export from index.ts, we rename it to "NewA"
export { A as NewA }

The .d.ts rollup will look like this:

export declare class NewA {
    static create(): NewA;
}

export { }

...but the .api.json for NewA.create() looks like this:

  {
    "kind": "Class",
    "canonicalReference": "api-documenter-test!NewA:class",  // <<== correct
    "excerptTokens": [
      {
        "kind": "Content",
        "text": "declare class "
      },
      {
        "kind": "Reference",
        "text": "A"
      },
      {
        "kind": "Content",
        "text": " "
      }
    ],
    "releaseTag": "Public",
    "name": "NewA",
    "members": [
      {
        "kind": "Method",
        "canonicalReference": "api-documenter-test!NewA.create:member(1)",  // <<== correct
        "docComment": "",
        "excerptTokens": [
          {
            "kind": "Content",
            "text": "static "
          },
          {
            "kind": "Reference",
            "text": "create"
          },
          {
            "kind": "Content",
            "text": "(): "
          },
          {
            "kind": "Reference",
            "text": "A",
            "canonicalReference": "!A:class"  // <<== incorrect
          },
          {
            "kind": "Content",
            "text": ";"
          }
        ],
        "isStatic": true,
        "returnTypeTokenRange": {
          "startIndex": 3,
          "endIndex": 4
        },
        "releaseTag": "Public",
        "overloadIndex": 1,
        "parameters": [],
        "name": "create"
      }
    ],
    "implementsTokenRanges": []
  }

Ideally the return type should be rewritten as:

    {
      "kind": "Reference",
      "text": "NewA",
      "canonicalReference": "api-documenter-test!NewA:class"
    },

[rbuckton]

[...] Ideally the return type should be rewritten as: [...]

What about cases like this?

class A {}
export { A as B, A as C };

What UID should use use for B and C? Do they end up with documentation entries that are just duplicates of A?

I'd rather the id for A be my-package!~A:class, since it is truly the canonical name. If you reference B or C as a type in your code, they should both link to A.

[octogonz]

What about cases like this?

class A {}
export { A as B, A as C };

What UID should use use for B and C? Do they end up with documentation entries that are just duplicates of A?

The .d.ts rollup currently handles it that way: Since A is not one of the exported names, then it's uncertain whether B or C should be the "official" name, so A is preserved locally in the rollup.

For the API documentation, it's not handled properly. (I don't remember the details, but it's something wrong like no class at all, or only one entry but with the wrong name A instead of B or C.) It's an important bug we should fix, but it hasn't been prioritized because it's somewhat rare.

This most commonly arises when an API got renamed, but they also want to preserve the old name to avoid breaking compatibility.

I'd rather the id for A be my-package!~A:class, since it is truly the canonical name. If you reference B or C as a type in your code, they should both link to A.

This is theoretically the best answer, but I'm not sure it corresponds well with how the language is used in real life. On the one hand, TypeScript is a general algebra that must describe every crazy thing you can do in untyped JavaScript. But on the other hand, when people design APIs and want to publish professional docs, it needs to be simple and understandable.

The story really should be: "This class is officially called B. But it has a secondary name C."

We could equivalently say: "The class A has an unreachable name. You must instead call it B or C when you import it. In the docs, we'll randomly use both names for no good reason. We won't call it A because some unrelated thing is exported with that name." This is theoretically identical, but it's a really terrible way to talk about your API. :-)

Thus, it might be better to find some way to designate an "official" name, and then rewrite the signatures to banish A from the .api.json file and .d.ts rollup.

[octogonz]

@rbuckton following up, yesterday we encountered a minor issue during testing. I emailed you the repro.

[octogonz]

Looks good. @rbuckton thanks again for getting this implemented -- it's a really cool feature!

[AlexJerabek]

Thanks @rbuckton and @octogonz. We really appreciate this effort.
I'm working through the Office-JS tooling pipeline to get the code snippets pointing at the new UIDs. Are there any obvious changes you can see to make in OfficeYamlDocumenter?

[octogonz]

FYI this was published as [email protected] and [email protected].

Are there any obvious changes you can see to make in OfficeYamlDocumenter?

Looking at the code, it seems like it should mostly work the same as before. There are a few places that match yamlItem.uid that might possibly need adjustments.

The way we validated SPFx was to generate docs in one folder using the old api-extractor and api-documenter (prior to Ron's change), and then in second folder using the new release. Then we used WinMerge to diff these two folders, and inspect how the .yml files had changed.

[octogonz]

(@AlexJerabek Unrelated, I recently started introducing a plugin-in model for api-documenter, which will eventually allow the OfficeYamlDocumenter customizations to be separated into their own package, so they can be decoupled from the main api-documenter code base. The more rigorous plugin contract should also reduce the likelihood of getting broken by code changes. If you're curious, there's an example plugin here, which gets invoked by this script which uses this config. I'll follow up with you later when this feature is more mature.)