Skip to content
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

[api-extractor] Linking to overloads generates warnings, but no way to specify which overload #1240

Closed
AlexJerabek opened this issue Apr 19, 2019 · 7 comments · Fixed by #1648
Labels
effort: medium Needs a somewhat experienced developer enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start!

Comments

@AlexJerabek
Copy link
Contributor

Our d.ts file has two namespaces named "Office". When running API Extractor version 7.1.0, links like this {@link Office.AsyncResult | AsyncResult} give warnings like this:

Warning: office.d.ts:2842:9 - (ae-unresolved-link) The @link reference could not be resolved: The reference is ambiguous because "Office" has more than one declaration; you need to add a TSDoc member reference selector

I wanted to use the guidance given in this TSDoc spec, but I'm unable to use the labels or index selectors to reference overloaded namespaces.

When using {@link (Office:2).AsyncResult}, I get the warning:

Warning: office.d.ts:328:5 - (ae-unresolved-link) The @link reference could not be resolved: The selector "2" is not a supported selector type

When using {@label COMMON_API}, I get the warning:

Warning: office.d.ts:7:5 - (tsdoc-unsupported-tag) The TSDoc tag "@Label" is not supported by this tool

Is there a workaround until labels or index-references are supported? Thank you.

@octogonz
Copy link
Collaborator

octogonz commented Apr 19, 2019

Distinguishing declarations is generally useful for merged declarations (e.g. overloaded functions, enum+namespace, interface+function, etc). In contrast, namespace+namespace and interface+interface are special cases where the distinction between declarations is NOT very meaningful.

If you have two namespaces named "Office", I suppose ideally API Extractor should collapse them into a single namespace. However this can involve nontrivial transformations, for example:

// "Oh, TypeScript!"  :-D
export namespace N {
  export interface A { }
  export interface C extends B { }
}

export namespace N  {
  export interface B { }
  export interface C extends A { }
}

The collapsed namespace would need a collapsed export interface C extends A, B { }.

For the special case of namespace+namespace (and interface+interface), I'm tempted to propose that the declaration reference resolver should arbitrarily select the first one it finds. In practice it doesn't matter much. This would eliminate the error and need for @label.

But @AlexJerabek... now I'm wondering how these multiple namespaces appear in your generated docs. Do they get folded together? Do they get separate TOC nodes? Does one of them get silently discarded? If the answer is something bad, then maybe we should go ahead and implement collapsing, at least in the .api.json file if not the .d.ts rollup.

(We also absolutely need to implement support for @label and :1. This isn't a lot of work. It was procrastinated merely because it didn't meet AE7's release bar of achieving feature parity with AE6. I don't believe it's needed for this case, but if you need it for overloaded functions, please open a separate GitHub issue to track it.)

@AlexJerabek
Copy link
Contributor Author

Thanks for the quick response @octogonz!

The namespaces get folded together today, which is what we'd expect. We can merge the namespaces together in our d.ts (apparently the separation was for historical reasons). That avoids this issue.

Because we're not blocked, this isn't an API Extractor feature we need. This issue was more to point out the discrepancy between the warning's advice and actual functionality.

@octogonz octogonz added the enhancement The issue is asking for a new feature or design change label Jun 2, 2019
@octogonz
Copy link
Collaborator

In #1308 @postspectacular wrote:

I understand this all is still in flux, but may I ask what is the currently supported syntax to link to an overloaded function? It seems there's some missing information and discrepancies between different parts of the documentation and the actual tooling/implementation. E.g. using the canonical references created by api-extractor in a {@link} tag cause the warnings for ambiguous references to disappear, but then still no link at all is showing up in the generated markdown...

Example:

Generated .api.json snippet:

{
  "kind": "Function",
  "canonicalReference": "@thi.ng/arrays!quickSort:function(1)",
  ...
}

Neither of these work (excluded from api-documenter markdown output):

  • {@link @thi.ng/arrays!quickSort:function(1)}
  • {@link @thi.ng/arrays#quickSort:function(1)}

Any (interim) advice is highly appreciated! Thanks!

@octogonz
Copy link
Collaborator

The short answer is that it's not implemented. (Fixing this is obviously a high priority in general, but embarrassingly it's stagnated because it doesn't affect us much; the Rush Stack project tries to avoid designing APIs with overloads.)

There are two versions of the @link syntax:

  • OLD: {@link @thi.ng/arrays#(quickSort:1)}
  • NEW: {@link @thi.ng/arrays!quickSort:function(1)}

The new notation was introduced this summer and has not been integrated yet into the TSDoc parser library. That work needs to be completed before API Extractor could adopt it (even though API Extractor is already using it for the "canonicalReference" ID).

Thus, if we want to get this working quickly, we should implement it for the old syntax. It's probably not a lot of work, and much of the implementation should be reusable later when we switch to the new syntax.

If I remember right, this needs to be implemented in two places:

Would someone want to help out with this? I'm pretty busy right now, so I'm not sure I could do it right away. But I'd be very happy to provide guidance/help if someone else wants to make the PR.

@octogonz octogonz added effort: medium Needs a somewhat experienced developer help wanted If you're looking to contribute, this issue is a good place to start! labels Nov 18, 2019
@octogonz
Copy link
Collaborator

Here's a PR: #1648

@postspectacular @AlexJerabek

@octogonz
Copy link
Collaborator

octogonz commented Dec 3, 2019

This fix was published with API Extractor 7.7.0

@postspectacular
Copy link

@octogonz you're a ⭐️ ! thank you so much & apols for lack of responses. busy times!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Needs a somewhat experienced developer enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants