[api-extractor] Incorrect handling of interfaces enhancing a class with the same name

[bajtos]

Please prefix the issue title with the project name i.e. [rush], [api-extractor] etc.

Is this a feature or a bug?

• [ ] Feature
• [x] Bug

Please describe the actual behavior.

Consider code that defines both a class and an interface with the same name. This approach is the recommended - if not the only - way how to describe events emitted by subclasses of Node.js EventEmitter, as explained on StackOverflow.

export class MyWidget extends EventEmitter {
  // class implementation goes here
}

export interface MyWidget {
  // `on()` and `once()` declarations for MyWidget events
}

The package api-extractor recognizes both artifact types and emits a class and an interface entry in the .api.json file.

When rendered using api-documenter, the index file show two entries with the same name (one in the list of classes, another in the list of interfaces). However, both entries point to the same markdown file that contains only members from the interface.

If the issue is a bug, how can we reproduce it? Please provide detailed steps and include a GitHub branch if applicable. Your issue will get resolved faster if you can make it easy to investigate.

I created a small repository reproducing the issue here: https://github.com/bajtos/bug-api-extractor

It contains all artifacts created by api-extractor and api-documenter. You can rebuild them yourself using npm run docs (just don't forget to npm install dependencies first).

What is the expected behavior?

Ideally, I'd like the interface entries to be merged into the class definition. From the point of view of an API consumer (the reader of API docs), there is a class extending EventEmitter that provides custom events with the necessary type information for the compiler. The fact that the events are described via an interface is IMO just an implementation detail.

If this is a bug, please provide the tool version, Node.js version, and OS.

• Tool: api-extractor
• Tool Version: 7.8.10
• Node Version: 14.3.0
  • Is this a LTS version? Not yet, but v14 will become LTS later this year
  • Have you tested on a LTS version? Yes, Node.js 12.16.2 produces the same outcomes.
• OS: MacOS

[raymondfeng]

It also happens for function and namespace that have the same name. Such as:

export function inject() {}

export namespace inject {
  export const tag = 'tag';
}

[octogonz]

However, both entries point to the same markdown file that contains only members from the interface.

This is a bug. It's related to https://github.com/microsoft/rushstack/issues/1308 and https://github.com/microsoft/rushstack/issues/1108. It's probably time to revisit this topic. I remember there was an open design question as to how to name the files to avoid collisions, but the actual fix should be very little work.

Ideally, I'd like the interface entries to be merged into the class definition. From the point of view of an API consumer (the reader of API docs), there is a class extending EventEmitter that provides custom events with the necessary type information for the compiler. The fact that the events are described via an interface is IMO just an implementation detail.

This should be a separate GitHub issue, because it is a feature request, not a bug. We could easily add hyperlinks to show that the class and interface are related. But I'm not sure how we could merge them into a single entry. In the navigation table-of-contents, they act as containers for child nodes -- would we mix the interface members together with the class members in a single list? That might be weird. If you have ideas, please open a separate issue where we can discuss that.

Whereas the "bug" is files overwriting each other.

[bajtos]

Thank you @octogonz for a detailed explanation and links to other relevant issues. ❤️

I am fine with your conclusion to fix file names collisions as a bug fix and then open another GitHub issue to discuss better handling of merged declarations as a new feature :+1:

[bajtos]

Workaround: mark the interface as @internal. api-extractor will complain about ae-different-release-tags, but the final markdown file will contain only the class members. The interface members will be hidden from the documentation, which may be ok in the particular case of describing EventEmitter events.

[bajtos]

Ideally, I'd like the interface entries to be merged into the class definition. From the point of view of an API consumer (the reader of API docs), there is a class extending EventEmitter that provides custom events with the necessary type information for the compiler. The fact that the events are described via an interface is IMO just an implementation detail.

This should be a separate GitHub issue, because it is a feature request, not a bug. We could easily add hyperlinks to show that the class and interface are related. But I'm not sure how we could merge them into a single entry. In the navigation table-of-contents, they act as containers for child nodes -- would we mix the interface members together with the class members in a single list? That might be weird. If you have ideas, please open a separate issue where we can discuss that.

Found a related discussion here: Handling/documenting merged declarations https://github.com/microsoft/tsdoc/issues/137 .

[kasperisager]

We're currently having to manually patch this by postfixing the basename with the item kind: https://github.com/Siteimprove/alfa/blob/6f7b28730c0a25d2448397f58d0ec164866dc2ba/.yarn/patches/api-documenter.patch#L22-L23. A proper fix would of course be much appreciated; is there anything I can do to help things along?

[maribethb]

My team is currently switching over to TypeScript from JS + Closure Compiler. We're using api-extractor and api-documenter to generate reference docs for our API and we are running into this problem as well, so I wanted to add my interest in a fix for this. In our case, we are using declaration merging with namespaces so we have a class and a namespace both called e.g. Block and the generated documentation only includes only the index page for the namespace.

This is pretty unfortunate for us. An easy solution on our end would be to not use namespaces like this, but since we're migrating an existing API it would be a breaking change for our users.

Earlier in the thread you mentioned

the actual fix should be very little work.

is that currently true and is something we could help with? Or is that dependent on the design issue in #1308 which is a bigger issue?