[api-extractor] incorrectly generates declarations for "export type" expression

[slavafomin]

Summary

Consider that you have some class defined in your library and you want to export it only as a "type", e.g. export type { SomeClass }. However, the API Extractor will ignore the type part and will generate the declarations with class available for use in the runtime:

// index.ts
export type { SomeClass } from './some-class';

// some-class.ts
export class SomeClass {}

// index.d.ts (wrong)
export declare class SomeClass {}

Expected behavior

Symbol exported using the export type construct should only be exported as a compile-only "type" and shouldn't look like it's available for use in runtime.

// index.d.ts (better)
declare class SomeClass {}
export type SomeClass = SomeClass;

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question	Answer
@microsoft/api-extractor version?	7.30.0
Would you consider contributing a PR?	No
TypeScript compiler version?	4.8.2

[Patrick-clone]

Hi, Is there any update on this?

[nick-pape]

Hey @iclanton , @pgonzal , @apostolisms , @johnguy0 ,

We encountered this issue internally in SharePoint today. This issue is particularly dangerous in cases where the package is externalized (or loaded from a CDN).

Since API-Extractor is not marking this as a type export in the rollup, it can be imported by consumers as a regular import. In the trivial case, this will be caught by webpack, which will complain that the import cannot be found in the .js file.

However, if you are using webpack externalization (or loading the JS from a CDN), you are not going to get a webpack error, you are going to get a runtime error. Which means you are relying on unit tests, integration tests, or monitoring to catch something that should be a build break.

This is particularly dangerous in the case of larger codebases, where someone may change a highly used export from a regular export to a type export, expecting to see a build error. And it may be unreasonable for them to test all the possible consuming scenarios. Additionally, its difficult/impossible to have runtime-based protection (i.e. kill switches) for a change like that.

[octogonz]

Is is a bug?

Yes, export type is a relatively new TypeScript feature that is not handled very well in this case.

Is it a priority?

From the context in this thread, this seems like NOT a best practice for API design. The goal seems to be to hide the constructor:

class ImportedX {
  public readonly title: string;
  public constructor(title: string) {
    this.title = title;
  }
}

export type X = ImportedX;

function test(a: X): void {
    // GOOD: We can access the members:
    console.log(a.title);

    // GOOD: And we can't access the constructor:
    // Error: 'X' only refers to a type, but is being used as a value here.(2693)
    const b: X = new X('my title');
}

...but other class aspects are lost as well:

    // BAD:
    // Error: 'X' only refers to a type, but is being used as a value here.(2693)
    if (a instanceof X) {
        console.log('success');
    }

...and most importantly, API Documenter cannot properly document the members, because it understands type X = ImportedX to be a "type" describing an anonymous (i.e. unexported) signature.

👉 An important principle of API Extractor is its perspective that APIs should be collections of "API items" that conform to straightforward stereotypes (class, interface, function, enum, etc). The compiler enables a sprawling (and often very confusing) landscape of possible type algebra, and API Extractor does more-or-less try to support that as much as possible. However, API Extractor's top priority is to establish intuitive API patterns that can each get their own web page and docsite navigation node. (The .d.ts file is not the docs!)

Is there a better approach (and workaround)?

If the goal is to prevent constructing the class, the best practice would simply be to make the constructor @internal:

export class X {
  public readonly title: string;

  /** @internal */
  public constructor(title: string) {
    this.title = title;
  }
}

// GOOD: Now this doesn't produce an error:
function example(a: X): void {
  if (a instanceof X) {
    console.log('success');
  }
}

Or if you want to operate within the type system, you could use a cast:

export class X {
  public readonly title: string;
  
  private constructor(title: string) {
    this.title = title;
  }
}

function someInternalCode(title: string): X {
  // Create an instance of X using a type cast to publicize the constructor:
  type NewX = { new(title: string): X };
  return new (X as unknown as NewX)(title);
}

How would we fix this bug though?

Instead of export type SomeClass = SomeClass, it looks like the .d.ts rollup can modify the export:

declare class SomeClass {
}
export { type SomeClass };

In this way, export type is modeled as a "kind of export" rather than as a "kind of declaration", and therefore the .api.json would still represent it as being a class API item. Approaching it this way may be relatively easy to implement, since we just need to remember whether type appeared in the import and adjust the export emitter:

https://github.com/microsoft/rushstack/blob/71bbe86ab301860092d666663f81a89c1b90dd24/apps/api-extractor/src/generators/DtsRollupGenerator.ts#L291-L293