[api-extractor] import() types are not handled correctly in .d.ts rollups

[alan-agius4]

Lazy types are not being rewired when using dtsRollup.

index.d.ts

export declare const ɵdefineDirective: import("./nested").CssSelector[];

nested.d.ts

export const enum SelectorFlags {
	NOT = 0b0001
}

export type CssSelector = (string | SelectorFlags)[];

output

export declare const ɵdefineDirective: import("./test-nested").CssSelector[];

export { }

I'd expect that the lazy type is resolved and appended to the file and the import is removed.

expected output

export const enum SelectorFlags {
	NOT = 0b0001
}

export type CssSelector = (string | SelectorFlags)[];
export declare const ɵdefineDirective: CssSelector[];

export { }

version: 7.0.11

[octogonz]

export declare const ɵdefineDirective: import("./nested").CssSelector[];

Oh man... do people really do this? :-P Is there a practical reason why this shouldn't be replaced by a conventional named import?

API Extractor should add support for this in theory, but I've been trying to triage people's feature requests into 3 categories:

1. Language features that are "recommended" according to our team's API design philosophy [100% supported]
2. All other commonly used language features, in their simplest syntax [AE7 represents major progress in this area]
3. Alternate syntaxes that can be replaced by an equivalent form from #2 [lowest priority, although people are encouraged to contribute PRs]

Inline import statements seem like they would definitely be in category #3.

[alan-agius4]

@pgonzal, the lazy import is not written in code, but emitted by the ts compiler.

Example having the following
nested.ts

export enum SelectorFlags {
	NOT = 0b0001
}

export type CssSelector = (string | SelectorFlags)[];

export function foo(): CssSelector {
	return;
}

index.ts

import { foo } from "./nested";

export const x = foo();

index.d.ts

export declare const x: (string | import("./nested").SelectorFlags)[];

While this can be trivial to fix and not rely on type inference, I'd at least expect that the dts bundler to emit some sort of error to warn the user.

[octogonz]

Ahh thanks for clarifying. This should be pretty easy to fix I think.

[octogonz]

BTW the "easy" fix is to report an error when this is encountered. Properly rewiring the import statements is not easy, since in general the referenced module may not be exported at all.

[Swatinem]

Hi! Author of rollup-plugin-dts here :-)

I just did a short comparison of some edge-cases between api-extractor and my plugin and noticed that your specific usecase might be supported by my plugin without a problem.
Feel free to try it and give some feedback :-)

[phryneas]

I believe we're also encountering that bug when trying to build https://github.com/async-library/react-async with pika, which internally uses api-extractor.

Typescript creates type files with import type references like state: AsyncState<T, import("./Async").AbstractState<T>>; and api-extractor then fails to inline those, resulting in a final file like this: https://gist.github.com/phryneas/d101e7e0b8070f6dc8b44540825c87cd#file-index-d-ts-L220 (line 220)

Could you take another look at this please?
(Original issue over at pika FredKSchott/pika-pack-builders#87 )

[octogonz]

@phryneas Can you share a repro branch that I could use to investigate/debug this?

[octogonz]

As far as the repro above, what's happening is that the type of x is not explicitly declared:

index.ts

import { foo } from "./nested";
export const x = foo();

Newer versions of the TypeScript compiler (>= 3.2) have started emitting the inferred type into the output, like this:

index.d.ts

export declare const x: (string | import("./nested").SelectorFlags)[];

The ugly-looking import("./nested") is generated I suppose because the compiler wants to avoid introducing a new local symbol in the file. (In some cases that might require renaming CssSelector to avoid a naming conflict.) To me this seems like not the best design choice. The compiler should either have left x as implicitly typed (which does not affect the accuracy of the type information at all), or else they should have gone all the way and generated a conventional import like this:

index.d.ts

import { CssSelector } from "./nested";
export declare const x: CssSelector;

In any case, our own projects would never encounter this issue, because we would never declare a public API without a type annotation. Our lint rules require the source code to look like this:

index.ts

import { foo, CssSelector } from "./nested";

export const x: CssSelector = foo();  // <--- properly typed variable

Compared to this, the original index.ts is arguably helping someone bang out code faster, at the expense of making the result more difficult for other people read and understand. Seems like great evidence in favor of this lint rule heheh.

API Extractor cannot emit import("./Async") in its output, so if we want to fix this, we will need to finish the job that (in my view) was really the compiler's responsibility. We can do that, but first I'd like to determine whether the compiler is already planning to address this in an upcoming release. (@rbuckton @RyanCavanaugh ?)

@phryneas In the meantime, are you able to work around your issue by adding explicit type annotations for the relevant APIs? If not, then we'd definitely give this fix a higher priority.

[phryneas]

You pointed me in the right direction to circumvent this bug - and pinpoint it in our case.
In case you're interested, here's the commit that still encounters the bug, but I'll explain it below:
https://github.com/phryneas/react-async/blob/0321c69f1bc4df61f89fa127045fbbb24add087f/packages/react-async/src/helpers.tsx

In this case, we have this method signature:

export const IfFulfilled = <T extends {}>({
  children,
  persist,
  state = {} as any,
}: {
  children?: FulfilledChildren<T>
  persist?: boolean
  state: AsyncState<T>
}) => (
  <>{state.isFulfilled || (persist && state.data) ? renderFn(children, state.data, state) : null}</>
)

Now, AsyncState takes an optional second parameter:

export declare type AsyncState<T, S extends AbstractState<T> = AbstractState<T>> = /* ... */

So the usage of AsyncState above has an implicit reference to AbstractState (which is not imported). And tsc doesn't leave it at that implicit reference, but makes it explicit by import()ing AbstractState, so something like this is generated AsyncState<T, import("./Async").AbstractState<T>>;.

And that leads to the whole problem.

Now, the solution is almost stupid: I only import AbstractState - I don't even need to use it. So, this diff for now solved all our problems: phryneas/react-async@70393af

But still, I guess that there are many more cases like this, where tsc will emit import() statements - I guess this issue will keep popping back up with the weirdest cases :/

[octogonz]

For references, this blog article describes some more use cases and motivation for "import() types" (which I think is the official name for this feature).

TypeScript's new import() types feature explained

[moltar]

Things break on this PR: ScaleLeap/amazon-mws-api-sdk#47

Things is, TS itself is generating this type. I have not written that manually.

export declare class Sellers {
    private httpClient;
    constructor(httpClient: HttpClient);
    listMarketplaceParticipations(): Promise<[MarketplaceParticipations, RequestMeta]>;
    listMarketplaceParticipationsByNextToken(nextToken: NextToken<'ListMarketplaceParticipations'>): Promise<[MarketplaceParticipations, RequestMeta]>;
    getServiceStatus(): Promise<[{
        Status: import("../parsing").ServiceStatus;
        Timestamp: string;
    }, RequestMeta]>;
}

[phryneas]

@moltar just as a workaround, do an import {ServiceStatus} from '../parsing' somewhere in the file that is coming from (you don't have to use it, it just has to be there) and it prevents the automatic import. Still not a good long-term solution though.

[javier-garcia-meteologica]

After implementing the error on import() type encounter, I thought about the next milestone: adding support for import() types.

The plan is to follow the same approach as rollup-plugin-dts.

// For import type nodes of the form
// `import("./foo").Bar`
// we create the following ESTree equivalent:
// 1. `import * as _ from "./foo";` on the toplevel
// 2. `_.Bar` in our declaration scope
convertImportTypeNode(node: ts.ImportTypeNode) {
...
}

First, _tryMatchImportDeclaration should find the import() type and create an AstImport, which is easy. Second, the rollup method _modifySpan should find again the import() type, lookup the previous AstImport and modify the Span with information derived from the AstImport (nameForEmit).

But I got stuck. The dtsRollup can only match Identifiers to AstImports, and import() types don't provide any Identifier. The core of this functionality is implemented at src/analyzer/AstSymbolTable.ts, in the function _fetchEntityForIdentifierNode().

Do two import() type statements with the same module path share the same Symbol?
E.g. import('abc').foo and import('abc').bar.

In that case AstSymbolTable can be modified to match Symbols to AstEntities. this._exportAnalyzer.fetchReferencedAstEntity() can already match Symbol -> AstEntities.

If this is implemented, identifiers could be matched to AstEntities using this scheme "Identifier -> Symbol -> AstEntity", or AstSymbolTable could have 2 tables: "Identifier -> AstEntity" and "Symbol -> AstEntity".

[octogonz]

Do two import() type statements with the same module path share the same Symbol?
E.g. import('abc').foo and import('abc').bar.

I would not rely on that assumption. For example in #1874 (comment) we recently found a violation of a reasonable assumption about ts.Declaration relationships that had always been true in the past.

(However if you follow the import to the imported module, that ts.Symbol is likely to be reliable for comparison. The machine that we normally use for this is TypeScriptInternals.getImmediateAliasedSymbol(). You can put a breakpoint here to see how it works. It is very similar to pressing F12 in VS Code -- it hops to the next alias along the way to the original declaration, allowing you to stop when you find a meaningful stopping point. I haven't tried it, but it would probably get you there, if the import() symbol itself doesn't already provide this info somehow. But see below.)

First, _tryMatchImportDeclaration should find the import() type and create an AstImport, which is easy.

Hmm... This is actually the part that needs an equivalence relation, to avoid creating duplicate AstImport instances. But it looks like we already do that a simpler way by using the import path string:

rushstack/apps/api-extractor/src/analyzer/ExportAnalyzer.ts

Lines 713 to 714 in 3bd05a0

	private _fetchAstImport(importSymbol: ts.Symbol | undefined, options: IAstImportOptions): AstImport {
	const key: string = AstImport.getKey(options);

...with:

rushstack/apps/api-extractor/src/analyzer/AstImport.ts

Lines 119 to 128 in aac158f

	public static getKey(options: IAstImportOptions): string {
	switch (options.importKind) {
	case AstImportKind.DefaultImport:
	return `${options.modulePath}:${options.exportName}`;
	case AstImportKind.NamedImport:
	return `${options.modulePath}:${options.exportName}`;
	case AstImportKind.StarImport:
	return `${options.modulePath}:*`;
	case AstImportKind.EqualsImport:
	return `${options.modulePath}:=`;

I bet this same idea will work for import()-type AstImport cases.

Second, the rollup method _modifySpan should find again the import() type, lookup the previous AstImport and modify the Span with information derived from the AstImport (nameForEmit).

The generator's lookup here is very dumb actually. It is essentially saying "Hey, remember when we already analyzed this thing in AstSymbolTable._analyzeChildTree()? Give me back the entity that we found then." So the mapping really should happen in _analyzeChildTree() long before the generator stage.

If there is no ts.Identifier, you could choose some other syntax element such as the import() node and generalize tryGetEntityForIdentifierNode() to accept either node type.

If this is implemented, identifiers could be matched to AstEntities using this scheme "Identifier -> Symbol -> AstEntity",

This might work, but it makes me uneasy. The idea behind _analyzeChildTree() is to force all the analysis to happen during a specific stage. For example, when the Collector goes and renames symbols, we wouldn't want new names suddenly getting analyzed into existence that we thought were available names. Relying on the syntax ts.Identifier (rather than the semantic ts.Symbol) is like an assertion that this specific thing was supposed to have been analyzed earlier. Using a semantic mapping might weaken that assertion and hide bugs.

Another issue is that the mappings can have tricky edge cases. For example, "displaced symbols" get filtered out during the mapping. We should be able to forget about all that nonsense after _analyzeChildTree() finishes its thing.

Again it might work. But I'd have to think about it to convince myself that it is correct for all cases. I'm not good at keeping lots of stuff in my head, so these proofs can be time-consuming (which is why #1796 is taking so long).

[octogonz]

That said, your overall idea sounds correct and rather straightforward to implement. I had assumed it would require some kind of architectural change. It would be so awesome to add support for import()!

I'm also reachable in the API Extractor chat room if you have more questions.

[javier-garcia-meteologica]

Thanks for your explanation @octogonz, it has been of great help. Now it's working, take a look at my branch import_type_support_simplicity.

Summary of how import() types are converted to star imports

• AstSymbolTable associates nodes (not only identifier nodes) to AstEntities.
  • _analyzeChildTree links ImportType nodes to AstEntities
• ExportAnalyzer has a new method fetchReferencedAstEntityFromImportTypeNode
  • For internal references, an AstSymbol is returned
  • For external references, an AstImport.StarImport is returned.
• tryGetEntityForIdentifierNode has been refactored into a generic tryGetEntityForNode
• _modifySpan replaces the whole ImportTypeNode with the AstSymbol.nameForEmit or builds a replacement token from a AstImport.StarImport

I also thought about another strategy: leave external references untouched as import() nodes if possible. The idea is to improve accuracy with respect to the original source. This attempt sits in branch import_type_support_accuracy.

It works, but there are some challenges to overcome. It is built on top of a new AstImport.ImportType and these are the challenges:

• Import() types may specify a namespace, such as import('y').a.b. Should it be exportName='a.b' exportName=['a', 'b'] or something else?
• The same external Symbol may be referenced twice in the project as an ImportType and as a NamedImport or as a StarImport. The problem in unifying them is that right now the first import discovered becomes the canonical AstEntity, but I would like to give preference to NamedImports or StarImports over ImportTypes, even if they are discovered afterwards.
• Great for rollups, but the AstImport.ImportType should be represented somehow in the api report. I thought about the following representation, but it is illegal

type foo = import('y').a.b;

[lifeiscontent]

@octogonz is anyone currently working on a fix for this?

[octogonz]

@lifeiscontent I believe that PR #1916 is a partial fix for this problem. Could you try that branch and see if it works for your scenario? We've started a new effort to get these old PRs finally merged, so if PR #1916 works for you, we could prioritize that one.

[lifeiscontent]

@octogonz is there a way I can npm install that PR?

[octogonz]

Instructions for compiling and running the branch are here:
https://api-extractor.com/pages/contributing/building/
https://api-extractor.com/pages/contributing/debugging/

If that doesn't work, tomorrow I can publish a PR build for you.

[octogonz]

@lifeiscontent We've published the PR branch as @microsoft/api-extractor@7.13.2-pr1916.0. Please give it a try.

[posva]

@microsoft/api-extractor@7.13.2-pr1916.0 works for vue-router-next!

[lishid]

@microsoft/api-extractor@7.13.2-pr1916.0 Can confirm, works for me too. 🎉

[jsamr]

@microsoft/api-extractor@7.13.2-pr1916.0 Not installable with yarn, because the shipped package.json contains dependencies with workspace scheme, which is non standard...

package.json

{
  "name": "@microsoft/api-extractor",
  "version": "7.13.2-pr1916.0",
  "description": "Analyze the exported API for a TypeScript library and generate reviews, documentation, and .d.ts rollups",
  "keywords": [
    "typescript",
    "API",
    "JSDoc",
    "AEDoc",
    "TSDoc",
    "generate",
    "documentation",
    "declaration",
    "dts",
    ".d.ts",
    "rollup",
    "bundle",
    "compiler",
    "alpha",
    "beta"
  ],
  "repository": {
    "type": "git",
    "url": "https://github.com/microsoft/rushstack/tree/master/apps/api-extractor"
  },
  "homepage": "https://api-extractor.com",
  "main": "lib/index.js",
  "typings": "dist/rollup.d.ts",
  "bin": {
    "api-extractor": "./bin/api-extractor"
  },
  "license": "MIT",
  "scripts": {
    "build": "heft test --clean"
  },
  "dependencies": {
    "@microsoft/api-extractor-model": "workspace:*",
    "@microsoft/tsdoc": "0.12.24",
    "@rushstack/node-core-library": "workspace:*",
    "@rushstack/rig-package": "workspace:*",
    "@rushstack/ts-command-line": "workspace:*",
    "colors": "~1.2.1",
    "lodash": "~4.17.15",
    "resolve": "~1.17.0",
    "semver": "~7.3.0",
    "source-map": "~0.6.1",
    "typescript": "~4.1.3"
  },
  "devDependencies": {
    "@rushstack/eslint-config": "workspace:*",
    "@rushstack/heft": "0.23.1",
    "@rushstack/heft-node-rig": "0.2.0",
    "@types/heft-jest": "1.0.1",
    "@types/lodash": "4.14.116",
    "@types/node": "10.17.13",
    "@types/resolve": "1.17.1",
    "@types/semver": "~7.3.1"
  }
}

[lishid]

@jsamr Yeah I ran into the same issue, ended up cloning the repo locally, fixing the package.json, and installing as a local dependency.

[elmpp]

@microsoft/api-extractor@7.13.2-pr1916.0 Not installable with yarn, because the shipped package.json contains dependencies with workspace scheme, which is non standard...

installable with Yarn1/2/3:

  "resolutions": {
    "@microsoft/api-extractor-model": "*",
    "@rushstack/node-core-library": "*",
    "@rushstack/rig-package": "*",
    "@rushstack/ts-command-line": "*",
    "@rushstack/eslint-config": "*"
  },

[octogonz]

Fixed by PR #1916 from @javier-garcia-meteologica

🎈 Released with API Extractor 7.18.0

[kasperisager]

That did the trick, thanks so much! 🙏