[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'