[api-extractor] just "don't export" @internal types instead of stripping them

[phryneas]

Is this a feature or a bug?

• Feature
• Bug

Please describe the actual behavior.

Currently, api-extractor strips all @internal types from a @public export.

What is the expected behavior?

I would like @internal types to still be present in the dts-rollup, but not exported - so other @public exported types can use them internally.

Reason: we have helper types that are exported because they are used throughout the project, but we don't want those to be exposed to our end users so we can change these in the future without being afraid of breaking things for library consumers. Having a ae-forgotten-export warning for those is fine, but currently the resulting types just reference types that aren't there - which makes them completely useless.

(And yes, those are in our entry point, because our entry point just does export * - after all, we monitor if we accidentally exported something we did not want to - thanks to api-extractor 😄. So we are just using @public and @internal to mark "what should be accessible for a library consumer". )

[octogonz]

What you described is the intended behavior. Maybe you found a bug? Could you share a repro?

[phryneas]

I've created a branch with the build artefacts of @reduxjs/toolkit here:

https://github.com/phryneas/redux-toolkit/tree/api-extractor-repo

Here is the entry point and this is the rollup

For example, ActionCreatorForCaseReducerWithPrepare is marked @internal and completely stripped, even though the @public CaseReducerActions relies on it. I would expect it to be just not exported, but present in the rollup.

[octogonz]

This seems like a bug. I was able to reduce your repro down to this:

/** @public */
export declare function f(x: X): void;

/** @internal */
export declare class X { }

...which produces a warning:

// Warning: (ae-incompatible-release-tags) The symbol "f" is marked as @public, but its signature references "X" which is marked as @internal

...but the resulting .d.ts rollup produces a compiler error:

/**
 * @public
 */
export declare function f(x: X): void; // <-- TS2304: Cannot find name "X"

/* Excluded from this release type: X */

I thought we had logic to handle this case, but apparently not. We would be unlikely to encounter this issue in our own projects, since we wouldn't allow ae-incompatible-release-tags for any of our public APIs. Probably that's why it was overlooked.

[phryneas]

I had feared as much - I seem to have a talent of uncovering api-extractor bugs ;)

I know this is a bit inappropriate to ask, but seeing that I don't have a smidgen of the understanding necessary to file a PR on this myself (heck, I don't even get it to build right now on my system): Can you give me a general ETA on when a fix for this could be landing in api-extractor?
The reason for me asking this is that I'd love to get a new version of @reduxjs/toolkit released before the end of the year, and depending on your answer I'd work around this by exporting types manually (instead of the wildcard export) for the time being, or wait for this to be fixed.
And yeah, holiday season is coming up, so essentially I fully expect you to say "not this year", but I guess asking won't hurt :)

[octogonz]

Fixing this compiler error makes sense. The TypeScript compiler today is much more strict about reporting errors in .d.ts files. Avoiding compile errors should be a higher priority than honoring trimming requests.

We could solve it like this: Before trimming a given @internal declaration, first consider every single reference to it, across the entire API surface. If any of those references come from a non-@internal declaration, then don't trim it. Maybe instead emit a comment saying that it couldn't be trimmed. This analysis can be performed during a single walk of the graph, so the performance cost should be minor. This algorithm seems sound and not too complex.

I would like @internal types to still be present in the dts-rollup, but not exported - so other @public exported types can use them internally.

@phryneas Here you are maybe asking for something extra: It sounds like you want API Extractor to change an exported declaration to be a local symbol, like this:

/** @public */
export declare function f(x: X): void;

/** @internal */
/* export */ declare class X { }

This is a more complicated request. It's almost like a different feature:

(And yes, those are in our entry point, because our entry point just does export * - after all, we monitor if we accidentally exported something we did not want to - thanks to api-extractor 😄. So we are just using @public and @internal to mark "what should be accessible for a library consumer". )

The original intended idea behind @internal was to support a collection of packages that build/ship together, to allow those packages to access each others' internals, while preventing "third parties" from accidentally consuming them.

It can also be used to hide file-level exports (e.g. APIs that are used by the internal implementation within a project). We've been considering to introduce a separate @private tag for this scenario, which would prevent those types from being exposed to other @internal projects. It seems a bit excessive, but in a large corporate monorepo, teams are concerned about clarifying contracts to avoid improper reliance on each other's implementation details, while still sharing "internals" that a third-party should not access.

But both scenarios could be handled by trimming (deleting declarations), not by undoing exports.

So coming back to your request: If you don't intend for others to consume the types, why export them from your library entry point? Is it simply a matter of convenience, that you want to write export * and then make API Extractor do the work of determining whether the API should really have been exported or not? I personally try to avoid export *, since it goes against the idea of being thoughtful about what is/isn't part of your library's public API.

In other words, is it an okay workaround to simply fix up your index.d.ts to export the right things?

[phryneas]

So coming back to your request: If you don't intend for others to consume the types, why export them from your library entry point? Is it simply a matter of convenience, that you want to write export * and then make API Extractor do the work of determining whether the API should really have been exported or not? I personally try to avoid export *, since it goes against the idea of being thoughtful about what is/isn't part of your library's public API.

In other words, is it an okay workaround to simply fix up your index.d.ts to export the right things?

Yeah, I guess this really is a convenience thing for us - so if this is really that complicated to implement, fixing up our index.d.ts is totally an option, too. Until now, the wildcard export had worked out pretty well since we have very few javascript exports and many typescript exports.

It was just something that I had assumed api-extractor would be doing anyways, so seeing it didn't do that, opening a feature request issue was the most obvious thing to do.
Api-extractor is one of maybe two or three options of doing dts-rollups out there, so I guess more libraries will start using it in the long run and have similar use cases.

[phryneas]

Oh, and generally: thanks for your super-fast reaction and looking into this. I really appreciate that 🤗

[octogonz]

If I get some more time over the holiday, I'll think about this algorithm some more. It may turn out to be relatively easy to implement, at least for the cases people are most likely to encounter.

thanks for your super-fast reaction and looking into this

It was an interesting question. :-)

[octogonz]

We could solve it like this: Before trimming a given @internal declaration, first consider every single reference to it, across the entire API surface. If any of those references come from a non-@internal declaration, then don't trim it. Maybe instead emit a comment saying that it couldn't be trimmed.

This algorithm leads to a graph theory problem:

• Each API declaration is a node in the graph
• For example, suppose function f returns interface Z. This forms an edge. We say f is a predecessor of Z in the graph; Z is an successor of f.
• The graph may have cycles, since two declarations can reference each other (e.g. X and Z below)
• The declared visibility of a node is its release tag of @internal, @alpha, @beta, @public (which we can replace with integers 1, 2, 3, 4)
• The transitive visibility for a node is the maximum visibility of all its predecessors

Example:

/** @internal=1 */
interface X {    // declared: 1, transitive: 4
  z: Z;
}

/** @alpha=2 */
interface Y {    // declared: 2, transitive: 2
  x: X;
}

/** @beta=3 */
interface Z {    // declared: 3, transitive: 4
  x: X;
}

/** @public=4 */
function f(): Z; // declared: 4, transitive: 4

Challenge: Given a graph and the declared visibility for each node, find an efficient algorithm to calculate the transitive visibility of all nodes in a graph.

[phryneas]

Hm. I'm not getting better than O(n²) here I believe.

• set the transitive visibility of all nodes to their declared visibility
• sort all nodes into one of four lists , depending on their current transitive visibility.
• for each list, (starting with 4) of transitive visibility TV
  • take the next item X
  • look at all successors of X.
    • if a successor has a lower transitive visibility, set it to TV and append the item to the current list, removing it from it's original
  • continue

This should be a "visit every node once, extending to each of their successors every time" algorithm,
so we're in O(n * [maximum numbers of successors of a node]) territory. I believe in extreme situations "maximum numbers of successors of a node" might be even higher than n, but for sanity I think it's a good assumption that it should almost never be >n, so this gives us a shoddy O(n²) - or in graph-terms: O(edges), which should be pretty optimal.

[octogonz]

Cool!

look at all successors of X.

How do you handle cycles?

so this gives us a shoddy O(n²) - or in graph-terms: O(edges), which should be pretty optimal.

Is that true? For example, what if the graph was 100 nodes organized into a linked list.

I posted this question on Twitter. Seems like there should be an O(nodes+edges) solution, similar to finding "strongly connected components" like with ES6 imports.

[phryneas]

Cool!

look at all successors of X.

How do you handle cycles?

Oh, I wasn't 100% clear on that: I only look at direct successors and never traverse down further.
Circles play no role here, since I look at every node in their role as "precedessor" only once and I finish looking at all "public" nodes and "nodes that have become public" before I look at the first "protected" node. There is no chance any node would be "widened" to public at a later point in time.

so this gives us a shoddy O(n²) - or in graph-terms: O(edges), which should be pretty optimal.

Is that true? For example, what if the graph was 100 nodes organized into a linked list.

Then we would look at every node once as a predecessor and once as a successor (except the first one of your linked list). 100 nodes, 99 edges, 199 "node visits". O(edges)=O(99)=O(nodes)=O(100)=O(nodes+edges)=O(199) - all in the same O-class :)

I posted this question on Twitter. Seems like there should be an O(nodes+edges) solution, similar to finding "strongly connected components" like with ES6 imports.

Assuming that count(edges) > count(nodes), O(edges + nodes) = O(edges). I just made that assumption. Of course, if count(edges) < count(nodes) we would be looking at O(nodes), but as that case seems unlikely (as I anticipate that count(edges) would be somewhere in count(nodes)² territory), I just wrote O(edges) above ;)

[phryneas]

Just for fun, an implementation that probably illustrates it better than I can describe it.

With your example of 100 nodes in a linked-list-like graph ;)

enum Level {
  internal,
  alpha,
  beta,
  public
}

interface GraphNode {
  declaredVisibility: Level
  transitiveVisibility: Level
  children: GraphNode[]
}

const buckets = [
  new Set<GraphNode>(), // internal
  new Set<GraphNode>(), // alpha
  new Set<GraphNode>(), // beta
  new Set<GraphNode>() // public
] as const

const nodes: Array<GraphNode> = []
for (let i = 0; i < 100; i++) {
  nodes.push({
    declaredVisibility: i % 4,
    transitiveVisibility: i % 4,
    children: []
  })
}

let lastNode: GraphNode | undefined
for (const node of nodes) {
  if (lastNode) lastNode.children.push(node)
  lastNode = node
}

// now nodes is a linked list of internal->alpha->beta->public->[repeat until 100 elements]

// let's begin the algorithm

// sort nodes into the four buckets
for (const node of nodes) {
  buckets[node.transitiveVisibility].add(
    Object.assign(node, { transitiveVisibility: node.declaredVisibility })
  )
}

let steps = 0

// walking through these outer two for-loops will visit each node exactly once since each node will exist in only one bucket at any given time.
for (let level = Level.public; level >= Level.internal; level--) {
  for (const predecessor of buckets[level].values()) { // values is an iterator, nodes added while in the loop will still be visited

    // now we visit the edges - as each node is only visited once, each directional edge of the graph is only visited once
    for (const successor of predecessor.children) {
      steps++
      if (successor.transitiveVisibility < level) {
        // add to the current bucket
        buckets[level].add(successor)
        // remove from the original bucket (which has not been visited yet)
        buckets[successor.transitiveVisibility].delete(successor)
        successor.transitiveVisibility = level
      }
    }
  }
}

console.log(
  buckets.map((set, level) => `${Level[level]}: ${set.size}`),
  `${steps} successor nodes visited`
)

//  [ 'internal: 1', 'alpha: 1', 'beta: 1', 'public: 97' ] '99 successor nodes visited'

[octogonz]

Ahh okay, from your original description I didn't understand that the algorithm should stop iterating when it reaches a successor with >= visibility:

    // now we visit the edges - as each node is only visited once, each directional edge of the graph is only visited once
    for (const successor of predecessor.children) {
      steps++
      if (successor.transitiveVisibility < level) {  // <=== 👍

This early exit, combined with the bucket sort, I think makes your algorithm actually O(nodes+edges) for worst case. Pretty good!

[ayZagen]

because of this I have to export all of my internal interfaces/classes and mark them as public. Is there any workaround ?

[phryneas]

Yeah, we got pretty lost in algorithmic fun there - @octogonz any chance that you can take a look at this?

[phryneas]

because of this I have to export all of my internal interfaces/classes and mark them as public. Is there any workaround ?

@ayZagen If you don't do an export * from ..., but export everything separately you should generally not have this problem.

[ayZagen]

@phryneas Thank you very much, I have removed exports and I had to change ae-forgotten-export logLevel to none. Otherwise command fails. For future comers in your api-extractor config you should have something like this

{
  "messages": {
    "extractorMessageReporting": {
      "ae-forgotten-export": {
        "logLevel": "none"
      }
    }
}

Exporting everything separately means lots of lines and would pollute the files in large project

[octogonz]

Yeah, we got pretty lost in algorithmic fun there - @octogonz any chance that you can take a look at this?

I very much would want to work on this. It would be fun to code. But other priorities have come along. For API Extractor, the priorities would be:

• several community PRs that are important features and need to get merged
• @rbuckton's big set of improvements for markdown support in TSDoc; I still haven't had time to get involved with that and feel pretty guilty about it 😇
• support for TypeScript 2.8 / 2.9

(If someone from the community wants to work on #1664, I would be happy to provide guidance.)

[adventure-yunfei]

Is there any progress here?

I've got another case: after trimming, some referenced unexported entity still exists in the dts rollup result. e.g.

declare interface A {}
export declare class B {
    /**
     * @internal
     */
    prop: A;
}

Which will produce:

declare interface A {}
export declare class B {
    /* Excluded from this release type */
}
export {}

The declaration "A" is meaningless. And I think finding visibility will solve this case, too.

[adventure-yunfei]

After trying some solutions, I'm thinking that this may not be the responsibility of api-extractor.

The purpose of keeping @internal types is: if we finally export a type (e.g. @public), we want to ensure that the type is valid with everything it refers existed.

The attempt of keeping @internal types just fixes one case. There're other cases, e.g. referring to inner property of a type. Take an example:

/** @internal */
export declare interface A {
    /** @internal */
    propA: number;
}
export declare class B {
    /**
     * @internal
     */
    prop: A['propA'];
}

To ensure type B is valid, we must keep both type A and its property propA. The first case would be solved here, but the second one is much more difficult. And there're so many ways to refer to inter types.

We might consider the first case as a normal one and support it, or we treat them the same: @internal/@beta trimming won't care who refers to it, its user's responsibility to mark @internal correctly to make the dts result valid.

[octogonz]

export declare class B {
    /**
     * @internal
     */
    prop: A['propA'];
}

This case feels fairly contrived. Although the TypeScript language provides some advanced syntaxes, arguably many of them are not good contracts for a public API signature.

declare interface A {}  // <--- implicitly @public
export declare class B {
    /**
     * @internal
     */
    prop: A;
}

Your above example seems different and should be its own GitHub issue: Because A has no release tag, it defaults to @public. And currently @public declarations are never trimmed from a .d.ts file, if they are reachable an exported type, even if that type is @internal. The _createEntityForIndirectReferences() analysis does not consider release tags at all. It was originally designed this way for simplicity, since API Extractor's general philosophy is that human API reviewers should be deciding what they want to support. Support should be explicit rather than determined by crawling references. But we could improve that, since the above behavior does seem a bit counterintuitive.

In summary, we have now discussed 3 distinct problems:

1. trimming by unexporting: This thread started with a feature request from @phryneas, for API Extractor to trim @internal types by removing the export keyword rather than completely deleting them.
2. ae-incompatible-release-tags without errors: I pointed out that maybe it's a bug to treat incompatible release tags as a "warning," yet produce a .d.ts file that doesn't compile. Instead, we could NOT trim @internal if it is referenced by a @public API.
3. trimming non-exported types differently depending on release type: For your example above.

1 and 2 require the new reachability algorithm proposed above. 3 could be solved by modifying the existing _createEntityForIndirectReferences() algorithm, and thus might be easier to implement.

As far as priority, I cannot say any of these would ever arise for the projects that I work on, given the way that we design APIs. But implementing a graph crawler sounds like a lot of fun!