[api-documenter] yaml validation error for static/instance methods with same name

[Vitalius1]

When running the api-documenter on the api.json file of the @uifabric/utilities package I get this error thrown:

When looking at the eventgroup.yml file generated I indeed see that they are the same:

... upon further digging into the package itself I found that the issue is from the EventGroup class has 2 methods with the name of raise. The only difference is that one is public static and the other one is just public.

https://github.com/OfficeDev/office-ui-fabric-react/blob/a1947a24c401056a470abcaef37694ae1c8419ef/packages/utilities/src/EventGroup.ts#L75
https://github.com/OfficeDev/office-ui-fabric-react/blob/a1947a24c401056a470abcaef37694ae1c8419ef/packages/utilities/src/EventGroup.ts#L321

Is this something that is a @uifabric/utilities package issue that needs to be addressed or api-documenter needs somehow to take that into the account?

For reference, this is what the api.json file has for both methods:

... and the error is most likely is thrown by this line in the typescript.schema.json:
https://github.com/Microsoft/web-build-tools/blob/5935e8e3d166d25777c0bd9e187267c29f6eb627/apps/api-documenter/src/yaml/typescript.schema.json#L64

cc: @natalieethell

[octogonz]

The DocFX yaml files require a unique uid to be assigned for each API item (see #881 (comment)).

A closely related problem is to provide a unique name and fullName, which gets displayed to the end user (see #1052 (comment)).

In both cases, we need a naming pattern that ideally would satisfy several requirements:

1. be unambiguous, i.e. provide a one-to-one mapping for all possible inputs
2. be stable, e.g. adding a new function overload should not cause the UID to change for a preexisting function overload (since that could lead to broken hyperlinks)
3. look "nice" to humans (particularly for the name field)

(Elsewhere I pointed out that the TSDoc declaration reference notation meets 1 and 2, but perhaps not 3.)

Examples of problematic TypeScript constructs:

• a static member with the same name as an instance member <-- your example above
• a function with multiple overloads
• a merged declarations (e.g. an enum X { ... } alongside namespace X { ... })
• an indexer (defined using symbols without any name) (e.g. [key: string] : string; as a member of an interface)

The hard part is deciding the right design. That's where the two linked issues got blocked. I haven't had a lot of time to think through this myself, but if you guys could propose a naming pattern, that will help get this resolved quickly. Actually updating the code in the _getUid() function is very easy.

@AlexJerabek (who helped with other DocFX design questions)

[Vitalius1]

Are there any limitations in the pattern of the naming? Meaning, are there any symbols or combinations of symbols that will or potentially create issues. Some ideas that come in my mind would be:
static vs instance: memberName_static, ClassName_memberName, memberName(static)
overloads: functionName_overload_1, functionName_1
no suggestions for the other 2 cases.

[JCMais]

note: comment written by someone that has never looked on Typescript AST and/or api-exporter output. So let me know if I get something wrong.

---

I think having _{number} as suffix for the overload methods would work, as suggested by @Vitalius1. From what I can see api-extractor give us an overloadIndex field that we can use.

I saw that the canonicalReference field is always unique, so we probably will need to use it. For reference here is the generated JSON for two symbols with the same name exported by a package: a class MyClass and a interface MyClass:

        {
          "kind": "Class",
          "canonicalReference": "(MyClass:class)",
          "excerptTokens": [ /* ... */ ],
          "releaseTag": "Public",
          "name": "MyClass",
          "members": [ /* ... */ ],
          "extendsTokenRange": { /* ... */ },
          "implementsTokenRanges": []
        },
        {
          "kind": "Interface",
          "canonicalReference": "(MyClass:interface)",
          "excerptTokens": [ /* ... */ ],
          "releaseTag": "Public",
          "name": "MyClass",
          "members": [ /* ... */ ],
          "extendsTokenRange": { /* ... */ }
        },

[octogonz]

This issue belongs to a family of closely related issues. I've created metaissue #1308 to come up with a unified fix that tackles them all together.

[Vitalius1]

@octogonz I’ve noticed there were a couple attempts in getting the #1308 issue fixed but it seems that waiting to agree on all problematic naming with typescript constructs is holding off the resolution of other ones that seem to be agreed on. We are currently blocked on overloaded functions and static vs instance naming for making our documentation project be on an automated pipeline. Maybe take the approach of having the list of all the scenarios and making fixes for each one of them and checking them out from the list?

[octogonz]

@Vitalius1 I'm not sure there's a shortcut. Here's where we stand:

• PR 1406 introduces a canonicalReference field in .api.json that correctly distinguishes static/instance members (as well as all the other cases such as function overloads). This is the difficult, important piece of technology that is the basis for solving all the ae-ambiguous-ids issues

• But PR 1406 is blocked by TSDoc PR 174 due to a bug in the new parser. This bug causes API Extractor to fail when processing @microsoft/rush-stack-compiler-3.5, so it is a serious issue that needs to be fixed. The fix requires a grammar change, so it's not super hard but not super easy either. Either @rbuckton or I will fix it as soon as we get a chance.

• Once those two PRs are merged, the solution to your fix is essentially already present in the PR 1337 branch. But that branch is fairly big, so we've been peeling off pieces into smaller PRs, rather than merging it directly. If you need a quick fix for [api-documenter] yaml validation error for static/instance methods with same name #1252, we could peel that off fairly easily.

So, in a nutshell, fixing TSDoc PR 174 is what will get things moving again. We need to prioritize that.

[Vitalius1]

@octogonz thanks for the detailed update. Will be patiently waiting for things to happen in the order you described. 👍

[Vitalius1]

cc: @aditima

[octogonz]

Status update:

• Today we merged and published PR Improve parse for module sources tsdoc#177 and PR [api-extractor] Add an experimental new ApiItem.canonicalReference property #1406
• Next I will do a relatively small PR that updates api-documenter to use the new canonicalReference field as the DocFX UID, and in .yaml filename; that should solve this issue (and maybe some others)
• Then I will merge all this recent work into @rbuckton's PR [api-extractor] Deeply linked types #1337 branch, and then we can finally move forward with the deep-linking feature

[octogonz]

PR #1450 fixes this (hopefully).