[api-extractor] Incompatible release tags across package boundaries yield invalid roll-ups

[Josmithr]

Summary

Consider the following example where a mono-repo contains 2 packages: Foo and Bar, structured as follows:

Package Foo

/**
 * @alpha
 */
export interface IFoo {...}

Package Bar (depends on Foo)

import { IFoo } from "Foo";

/**
 * @public
 */
export interface IBar extends IFoo {...}

API-Extractor will generate an invalid public d.ts rollup for package Bar that looks like the following:

/* Excluded from this release type: IFoo*/
/**
 * @public
 */
export declare interface IBar extends IFoo {}

This isn't valid. At a minimum I would expect the following in the generated d.ts file:

import { IFoo } from "Foo";

Standard questions

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

Question	Answer
@microsoft/api-extractor version?	7.38.3
Operating system?	Linux
API Extractor scenario?	rollups (.d.ts)
Would you consider contributing a PR?	No
TypeScript compiler version?	5.1.6
Node.js version (node -v)?	18.17.1

[octogonz]

This sounds closely related to #3521 which @Josmithr is working on. There are some design notes in https://rushstack.io/community/event/?id=54

[Josmithr]

@octogonz I filed both of these :) I don't think they're related, or if they are I'm missing something. This issue is about the generated d.ts files, not docs.

[octogonz]

Sorry, I was trying to respond to some issues using my phone and got confused jumping between apps. 😆

If I understand your issue description correctly, it sounds like simply a bug: An @alpha tag in a different package should not affect the .d.ts trimming for the current package. (You did not mention bundledPackages here, so I assume that is not required for this repro.)

[octogonz]

Could you share a repro?

[Josmithr]

@octogonz I set up a quick mono-repo environment that reproduces the issue pretty minimally. The README contains the repro steps.
https://github.com/Josmithr/cross-package-trimmed-rollup-bug-repro

Note that bundledPackages is not required to repro this.

[Josmithr]

I went ahead and reduced the scope of the issue here, since I think my original description was inadvertently coupling 2 independent issues. This issue is strictly about the invalid d.ts files that are generated.

I filed #4430 to track a related issue where ae-incompatible-release-tags across package boundaries with bundledPackages enabled.

[Josmithr]

I wonder if this is related? The comment seems to indicate that it is desired to trim imports from other packages based on their release tags, which seems inherently incorrect.

[octogonz]

Thanks for this helpful repro. It is definitely a bug that API Extractor is producing a rollup that does not compile.

I wonder if this is related?

Yes.

The comment seems to indicate that it is desired to trim imports from other packages based on their release tags, which seems inherently incorrect.

In the context of your example, I agree that it seems incorrect to be trimming based on an @internal comment that is NOT part of project-b (not even via the bundledPackages feature).

To get into the design details, let's consider a situation like this:

• IBetaThing is marked @beta
• publicFunc() is marked @public, that accepts a parameter of type IBetaThing
• We are generating a "public" rollup, which trims @beta and @internal declarations

/** @beta */
export interface IBetaThing { }

/** @public */
export function publicFunc(param: IBetaThing): void { }

Thought experiment 1

Suppose these APIs were initially part of a single package. In this case we would expect API Extractor to:

1. Report ae-incompatible-release-tags, warning that publicFunc() should not depend on @beta declarations (because a trimmed caller wouldn't have any way to construct that type), AND
2. Due to this warning, the trimming would preserve IBetaThing as an unexported API so that the code still compiles.

I'm sure we had implemented that second part in the past, but it seems to be broken (maybe it got lost in the version 7 overhaul?). The .d.ts rollup is already broken:

rollup-public.d.ts

/* Excluded from this release type: IBetaThing */

/** @public */
export declare function publicFunc(param: IBetaThing): void;

export { }

Thought experiment 2

Okay now suppose that we extract publicFunc() into package-b, which depends on package-a containing IBetaThing. (And let's assume bundledPackages=false; as bundling is its own world of edge cases.)

On the surface, in a large scale professional scenario, we would expect to be able to divide up our API into multiple packages without changing the semantics of @beta. Specifically, we would want ae-incompatible-release-tags analysis to be performed across our packages. But that raises a couple concerns:

• Is @beta really an AEDoc tag for package-a, or just someone's JSDoc comment that happens to look like AEDoc? This concern is supposed to be addressed by the tsdoc-metadata.json file generated by API Extractor and published with the NPM package.

• Can package-a be trimmed differently from package-b? And even if they are the same, how would a @beta API from package-b import a @beta API from package-a to produce rollup-beta.d.ts? In the use case that originally motivated this feature, Microsoft was shipping a customer SDK comprised of many NPM packages that get built/published as a collection (with identical NPM version numbers). These packages accessed each other's internals, which meant that in the monorepo workspace, the package.json files specify "typings": "dist/index-internal.d.ts". The @beta release was published using a special NPM version; IIRC after compiling and before publishing, the package.json would get rewritten to substitute "typings": "dist/index-public.d.ts". In this way, the finally shipped .d.ts files had consistent trimming, whereas the underlying build process had full access to internals.

Taken together, these two solutions correctly preserve the ae-incompatible-release-tags feature and semantics of @beta across a set of multiple packages, but Microsoft's approach involves a number of assumptions that must be carefully coordinated. Maybe we should consider it an "advanced" model.

It can be contrasted with a "basic" model, where we simply forbid package-a to import trimmed members from package-b. Instead we might say that @beta only has meaning within package-b, and package-a should consider any declaration outside of its own package to be @public regardless of what its TSDoc might say.

Does that make sense? If so, perhaps we could proceed by inventing a name for these two models, and an api-extractor.json setting to select the behavior, and probably the "basic" model should be the default.

[Josmithr]

This concern is supposed to be addressed by the tsdoc-metadata.json file generated by API Extractor and published with the NPM package.

I wasn't actually aware of this behavior. I knew that file could be generated, but had no idea it influenced how api-extractor might work in other packages. This makes sense to me now, but it might be useful feedback that I was not aware of this after having gone through the website docs a fair number of times. Might be worth an audit of the website to ensure this is clear for consumers 🙂

I think I'm understanding the situation a bit better, though, so thank you for the thorough explanation / thoughts.

Is @beta really an AEDoc tag for package-a, or just someone's JSDoc comment that happens to look like AEDoc? This concern is supposed to be addressed by the tsdoc-metadata.json file generated by API Extractor and published with the NPM package.

This seems like a critical bug. Any idea what the fix needs to be? Is there someone who could pick up this work?

Would a reasonable short-term mitigation be to simply not trim imports specifically?

It can be contrasted with a "basic" model, where we simply forbid package-a to import trimmed members from package-b. Instead we might say that @beta only has meaning within package-b, and package-a should consider any declaration outside of its own package to be @public regardless of what its TSDoc might say.

Does that make sense? If so, perhaps we could proceed by inventing a name for these two models, and an api-extractor.json setting to select the behavior, and probably the "basic" model should be the default.

Yes, this makes sense to me, and the basic model captures my assumptions about what was supposed to be happening.

I also think this is critical to resolve, as I know of a few teams that are using api-extractor, and each is operating with their own release tag semantics, and I don't believe they are aware that there is an implicit assumption that the semantics match in any way (even if only when publishing the tsdoc-metadata report).

[Josmithr]

@octogonz based on our conversation today, I have put up a PR that fixes the issue by simply never trimming import statements when generating rollups: #4498