[api-extractor] UnhandledPromiseRejectionWarning

[lizozom]

Please prefix the issue title with the project name i.e. [rush], [api-extractor] etc.

Is this a feature or a bug?

• Feature
• Bug

Please describe the actual behavior.

Trying to build docs for Kibana, results in the following error:

(node:8706) UnhandledPromiseRejectionWarning: Error: Internal Error: Cannot assign isExternal=true for the symbol React because it was previously registered with isExternal=false

If the issue is a bug, how can we reproduce it? Please provide detailed steps and include a GitHub branch if applicable. Your issue will get resolved faster if you can make it easy to investigate.

Checkout this KIbana branch https://github.com/lizozom/kibana/tree/newplatform/docs-test
Run yarn kbn bootstrap to install dependencies
Run node scripts/check_core_api_changes.js to start generating the docs.
After ~1min you'll see the error mentioned.

What is the expected behavior?
Docs should be generated

If this is a bug, please provide the tool version, Node.js version, and OS.

• Tool:
• Tool Version:
• Node Version: 10.19.0
  • Is this a LTS version?
  • Have you tested on a LTS version?
• OS: Ubuntu 18.04

[octogonz]

Since you were interested in improving the API Extractor diagnostics, here's some notes about how I debugged it.

First I created a VS Code debug configuration like this:

launch.json

{
    // Use IntelliSense to learn about possible attributes.
    // Hover to view descriptions of existing attributes.
    // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
    "version": "0.2.0",
    "configurations": [
        {
            "type": "node",
            "request": "launch",
            "name": "Launch Program",
            "program": "${workspaceFolder}/scripts/check_core_api_changes.js"
        }
    ]
}

The VS Code debugger stopped in AstSymbolTable.ts on line 548 where InternalError was thrown. Because InternalError is only used to report bugs, it automatically calls debugger; which make debugging nice and easy.

AstSymbolTable.ts

    if (options.isExternal !== astSymbol.isExternal) {
      throw new InternalError(`Cannot assign isExternal=${options.isExternal} for`
        + ` the symbol ${astSymbol.localName} because it was previously registered`
        + ` with isExternal=${astSymbol.isExternal}`);
    }

In the debug console, I looked at astSymbol...

astSymbol._astDeclarations[0].declaration.getSourceFile().fileName

...which shows it is talking about @types/react/index.d.ts. To find the location, do this:

astSymbol._astDeclarations[0].declaration.getSourceFile().pos

That tells us character position is 2671. To translate it to a line number, I did this:

astSymbol._astDeclarations[0].declaration.getSourceFile().getLineAndCharacterOfPosition(2671)

...which points us to this export:

@types/react/index.d.ts

export = React;

That is not the modern good way to import React. Some code is probably mixing together import react = require("react") with import React from "react";. API Extractor cannot easily normalize these in the .d.ts rollup (due to complexities of TypeScript that I won't get into), so API Extractor really needs all imports of React to be done in a consistent way (for the API surface at least).

Regardless, API Extractor assigned astSymbol.isExternal to false for this import, which is not right. That is the bug we need to fix. It's not too surprising since the code bases we test with probably always import React correctly.

Before moving on though, let's figure out which file has the bad import, because fixing that may provide a workaround.

If we go up the call stack a couple records, the call is coming from this code:

ExportAnalyzer.ts

      } else if (declaration.kind === ts.SyntaxKind.ImportClause) {
        // EXAMPLE:
        // "import A, { B } from './A';"
        //
        // ImportDeclaration:
        //   ImportKeyword:  pre=[import] sep=[ ]
        //   ImportClause:  <------------- declaration (referring to A)
        //     Identifier:  pre=[A]
        //     CommaToken:  pre=[,] sep=[ ]
        //     NamedImports:
        //       FirstPunctuation:  pre=[{] sep=[ ]
        //       SyntaxList:
        //         ImportSpecifier:
        //           Identifier:  pre=[B] sep=[ ]
        //       CloseBraceToken:  pre=[}] sep=[ ]
        //   FromKeyword:  pre=[from] sep=[ ]
        //   StringLiteral:  pre=['./A']
        //   SemicolonToken:  pre=[;]

        const importClause: ts.ImportClause = declaration as ts.ImportClause;
        const exportName: string = importClause.name ?
          importClause.name.getText().trim() : ts.InternalSymbolName.Default;

        if (externalModulePath !== undefined) {
          return this._fetchAstImport(declarationSymbol, {
            importKind: AstImportKind.DefaultImport,
            modulePath: externalModulePath,
            exportName
          });
        }

Printing importClause.getSourceFile().fileName shows that this import is from search_bar/search_bar.d.ts which does a modern React import. So where did that = import come from? What's happening is that _fetchAstSymbol() is finding a cached result from an earlier import of this same symbol.

AstSymbol does not remember who introduced it. (Maybe we could improve that?) We can intercept the initial cache miss, though. I put a VS Code breakpoint on this line:

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

Lines 511 to 513 in 64ebf51

	astSymbol = new AstSymbol({
	followedSymbol: followedSymbol,
	localName: localName,

To avoid breaking on every single symbol, I changed it to be a conditional breakpoint looking for the expression localName === 'React'. Then I restart the process and wait for it to break. The call stack originates from a big _collectAllExportsRecursive() visitor. The immediate caller is AstSymbolTable._analyzeChildTree() and syntax it's analyzing is in the node parameter to that function. Printing node.getSourceFile().fileName takes us to this file:

kibana/target/types/plugins/data/public/types.d.ts

/// <reference types="react" />
import { CoreStart } from 'src/core/public';
import { IStorageWrapper } from 'src/plugins/kibana_utils/public';
import { UiActionsSetup, UiActionsStart } from 'src/plugins/ui_actions/public';
. . .

/// <reference types="react" /> is the problem! The expressions like React.ComponentType<IndexPatternSelectProps> are accessing React via a global variable rather than properly importing it. Fixing that should give you your workaround. And now we have a clue why API Extractor set isExternal=false. From #1316 I remember that the compiler's API makes it somewhat tricky to determine where global variables come from.

Let me investigate that a bit more and follow up.

[octogonz]

Returning to the above example, the analyzer encounters React in a context like this:

kibana/target/types/plugins/data/public/types.d.ts

/// <reference types="react" />
. . .
export interface DataPublicPluginStart {
    . . .
    ui: {
        IndexPatternSelect: React.ComponentType<IndexPatternSelectProps>;
        SearchBar: React.ComponentType<StatefulSearchBarProps>;
    };
}

When it sees the React symbol, it calls TypeScriptInternals.getImmediateAliasedSymbol() repeatedly to hop along a chain of import statements (similar to pressing F12 in VS Code) until it reaches the original class definition. If one of these import statements refers to an NPM package, that's how we know isExternal=true, i.e. the React.ComponentType class isn't something from the local project's source files. API Extractor devotes a lot of logic to this hopping because the compiler doesn't tell us about it. (It's not even an "internal" compiler API that we can secretly call, but instead trapped inside local variables and function closures that are impossible to access without modifying the compiler itself.)

But when React is a global variable, the analyzer teleports directly into @types/react/index.d.ts without traversing any import statement. Thus it seems to be a local declaration.

The import hopping logic is here:

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

Lines 330 to 366 in 5627415

	public fetchReferencedAstEntity(symbol: ts.Symbol, referringModuleIsExternal: boolean): AstEntity | undefined {
	let current: ts.Symbol = symbol;
	if (referringModuleIsExternal) {
	current = TypeScriptHelpers.followAliases(symbol, this._typeChecker);
	} else {
	for (; ;) {
	// Is this symbol an import/export that we need to follow to find the real declaration?
	for (const declaration of current.declarations || []) {
	let matchedAstEntity: AstEntity | undefined;
	matchedAstEntity = this._tryMatchExportDeclaration(declaration, current);
	if (matchedAstEntity !== undefined) {
	return matchedAstEntity;
	}
	matchedAstEntity = this._tryMatchImportDeclaration(declaration, current);
	if (matchedAstEntity !== undefined) {
	return matchedAstEntity;
	}
	}
	if (!(current.flags & ts.SymbolFlags.Alias)) { // eslint-disable-line no-bitwise
	break;
	}
	const currentAlias: ts.Symbol = TypeScriptInternals.getImmediateAliasedSymbol(current, this._typeChecker);
	// Stop if we reach the end of the chain
	if (!currentAlias || currentAlias === current) {
	break;
	}
	current = currentAlias;
	}
	}
	// Otherwise, assume it is a normal declaration
	const astSymbol: AstSymbol | undefined = this._astSymbolTable.fetchAstSymbol({

But debugging reveals that it starts off already inside @types/react/index.d.ts. The initial hop occurs in AstSymbolTable._analyzeChildTree() here:

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

Lines 318 to 323 in 5627415

	const symbol: ts.Symbol | undefined = this._typeChecker.getSymbolAtLocation(identifierNode);
	if (!symbol) {
	throw new Error('Symbol not found for identifier: ' + identifierNode.getText());
	}
	referencedAstEntity = this._exportAnalyzer.fetchReferencedAstEntity(symbol,

The this._typeChecker.getSymbolAtLocation(identifierNode) already returns a symbol whose declaration is inside @types/react/index.d.ts, specifically the export = React; line. Without an import statement, AstSymbolTable.fetchAstSymbol() concludes that @types/react/index.d.ts is part of the local project.

Now @types/react is pretty obviously an external package, but we cannot use a simple heuristic like looking for the @types substring. API Extractor needs a rigorous solution that will handle all the weird ways this bug can arise.

Normally when the analyzer traverses into an external module, it stops and returns an AstImport instead of an AstSymbol. This ensures that the import clause gets properly emitted in the .d.ts rollup. Thus the 100% correct way to model global variables would be to introduce a new kind of AstImport that represents the /// <reference types="react" /> form. This could also give us the missing information needed to eliminate a minor loophole, where API Extractor emits unused reference lines if they appeared in the same file as an exported API. But that would be nontrivial change and maybe not worth the effort.

A simpler solution would be to ignore the React global variable. The reference will get emitted anyway, and if it participates in declaration renaming (another important scenario for AstSymbol objects) it can be handled generically like other globals. But how to detect this case? For example, we don't want to accidentally detect React symbols that came from a normal import statement (i.e. that replaces the global variable in that source file).

I'm thinking of two alternative ways to approach this:

1. Look for cases where typeChecker.getSymbolAtLocation() or TypeScriptInternals.getImmediateAliasedSymbol() unexpectedly takes us to a different source file without hopping via an import statement. If it involves a global variable name, then we can assume it's external. (If not, it's probably an API Extractor bug.) The [api-extractor] .d.ts rollup sometimes emits names that conflict with global names #1095 discussion gave some approaches for detecting globals.

2. Alternatively, we could look at the symbol's ts.SourceFile and try to get the compiler to tell us whether it was external or not. Historically this was tricky because the compiler doesn't strongly distinguish files as being "external" or not. It treats NPM packages as a way to locate source files, not as a partitioning of code into modules. But maybe things have improved.

(There is also the problem TypeScript permits a single symbol to have merged declarations coming from multiple source files, but we'll continue to consider that out of scope for now heheh.)

[octogonz]

Oops I actually forgot about the original UnhandledPromiseRejectionWarning message from Node.js. That is actually due to a bug in the kibana script that invokes API Extractor:

src/dev/run_check_core_api_changes.ts

(async () => {
  . . .
  const results = await Promise.all(
    Object.keys(folderMapping).map(srcFolder =>
      run(srcFolder, folderMapping[srcFolder], { log, opts })
    )
  );

  if (results.find(r => r === false) !== undefined) {
    process.exitCode = 1;
  }
})();  // <=== MISSING .catch() HANDLER HERE

@lizozom You guys should definitely fix that and enable the @typescript-eslint/no-floating-promises lint rule.

[octogonz]

Here's a sketch of an implementation for idea #1 above: PR #1767

[lizozom]

@octogonz thank you so much for the detailed response.
First of all - importing React in the file you had found indeed fixes the problem.
I will try implement the debugging stages you suggested to make sure I'm able to detect the problem myself in the next couple of days and update!

[octogonz]

Fixed by PR #1767 which was published with API Extractor 7.7.12.

[octogonz]

BTW I've also improved --diagnostics switch to report when globals are encountered, which will show the path of which file is referencing the React global.