microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.92k stars 595 forks source link

[api-extractor] Unable to analyze the export #2220

Open larshp opened 4 years ago

larshp commented 4 years ago

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

Is this a feature or a bug?

Please describe the actual behavior. api-extractor 7.9.19 - https://api-extractor.com/

Using configuration from C:\Users\larsh\git\mysql\api-extractor.json

Analysis will use the bundled TypeScript version 3.9.7
*** The target project appears to use TypeScript 4.0.3 which is newer than the bundled compiler engine; consider upgrading API Extractor.

Error: Internal Error: Unable to analyze the export "ConnectionOptions" in
C:/Users/larsh/git/mysql/lib/Connection.d.ts

You have encountered a software defect. Please consider reporting the issue to the maintainers of this application.
    at ExportAnalyzer._getExportOfAstModule (C:\Users\larsh\git\mysql\node_modules\@microsoft\api-extractor\lib\analyzer\ExportAnalyzer.js:471:19)
    at ExportAnalyzer._getExportOfSpecifierAstModule (C:\Users\larsh\git\mysql\node_modules\@microsoft\api-extractor\lib\analyzer\ExportAnalyzer.js:464:32)
    at ExportAnalyzer._tryMatchImportDeclaration (C:\Users\larsh\git\mysql\node_modules\@microsoft\api-extractor\lib\analyzer\ExportAnalyzer.js:383:29)
    at ExportAnalyzer.fetchReferencedAstEntity (C:\Users\larsh\git\mysql\node_modules\@microsoft\api-extractor\lib\analyzer\ExportAnalyzer.js:251:45)
    at ExportAnalyzer._tryGetExportOfAstModule (C:\Users\larsh\git\mysql\node_modules\@microsoft\api-extractor\lib\analyzer\ExportAnalyzer.js:496:34)
    at ExportAnalyzer._getExportOfAstModule (C:\Users\larsh\git\mysql\node_modules\@microsoft\api-extractor\lib\analyzer\ExportAnalyzer.js:469:32)
    at C:\Users\larsh\git\mysql\node_modules\@microsoft\api-extractor\lib\analyzer\ExportAnalyzer.js:216:60
    at Map.forEach (<anonymous>)
    at ExportAnalyzer._collectAllExportsRecursive (C:\Users\larsh\git\mysql\node_modules\@microsoft\api-extractor\lib\analyzer\ExportAnalyzer.js:207:48)
    at ExportAnalyzer.fetchAstModuleExportInfo (C:\Users\larsh\git\mysql\node_modules\@microsoft\api-extractor\lib\analyzer\ExportAnalyzer.js:157:18)

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. Steps to reproduce,

git clone https://github.com/larshp/mysql
npm install
npm run bundle

What is the expected behavior? types are bundled

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

benmccann commented 3 years ago

I got this as well when setting "bundledPackages": ["connect"], in Vite. Using api-extractor version 7.18.5. Stacktrace refers to the same method (new line number since new version):

You have encountered a software defect. Please consider reporting the issue to the maintainers of this application.
    at ExportAnalyzer._getExportOfAstModule (node_modules/@microsoft/api-extractor/lib/analyzer/ExportAnalyzer.js:595:19)
    at ExportAnalyzer._getExportOfSpecifierAstModule (node_modules/@microsoft/api-extractor/lib/analyzer/ExportAnalyzer.js:588:32)
    at ExportAnalyzer._tryMatchImportDeclaration (node_modules/@microsoft/api-extractor/lib/analyzer/ExportAnalyzer.js:512:29)
    at ExportAnalyzer.fetchReferencedAstEntity (node_modules/@microsoft/api-extractor/lib/analyzer/ExportAnalyzer.js:288:45)
    at AstSymbolTable._analyzeChildTree (node_modules/@microsoft/api-extractor/lib/analyzer/AstSymbolTable.js:328:76)
    at AstSymbolTable._analyzeChildTree (node_modules/@microsoft/api-extractor/lib/analyzer/AstSymbolTable.js:373:18)
    at AstSymbolTable._analyzeChildTree (node_modules/@microsoft/api-extractor/lib/analyzer/AstSymbolTable.js:373:18)
    at AstSymbolTable._analyzeChildTree (node_modules/@microsoft/api-extractor/lib/analyzer/AstSymbolTable.js:373:18)
    at AstSymbolTable._analyzeAstSymbol (node_modules/@microsoft/api-extractor/lib/analyzer/AstSymbolTable.js:243:18)
octogonz commented 3 years ago

I was able to reproduce the error using your https://github.com/larshp/mysql branch, thanks!

However, the .d.ts inputs are very weird. The APIs are exported using the ancient export = form that is now deprecated:

mysql/lib/Connection.d.ts

. . .
declare namespace Connection {

    export interface ConnectionOptions {
        . . .
    }

    export interface SslOptions {
        . . .
    }
}

declare class Connection extends EventEmitter {
        . . .
}

export = Connection;

...but your entry point does not import it using the corresponding import Connect = require('./lib/Connection') syntax. Nor does it use the more modern esModuleInterop style of import * as Connect from './lib/Connection'. Instead, it tries to import them like named exports:

mysql/index.d.ts

import {ConnectionOptions, SslOptions} from './lib/Connection';

I'm not sure whether that is even valid syntax or not, but it's definitely not an approach that we've tested for API Extractor.

Could you eliminate the export = from Connection.d.ts and replace it with standard ECMAScript exports? That would be the best solution for this problem.

benmccann commented 3 years ago

Thanks so much for tracking the issue down! I just tried changing the DefinitelyTyped package and it does fix the api-extractor issue, but unfortunately fails the DefinitelyTyped tests when I run npm test connect

The declaration doesn't match the JavaScript module 'connect'. Reason: The declaration should use 'export =' syntax because the JavaScript source uses 'module.exports =' syntax and 'module.exports' can be called or constructed.

To learn more about 'export =' syntax, see https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require.

Here's their code: https://github.com/senchalabs/connect/blob/5c52e388feaa914a5a054fac21c4f4c6698a64c4/index.js#L28

I'm not sure if that code should be updated though? It seems like a valid thing to do and am not sure how I'd change the actual senchalabs/connect package

octogonz commented 3 years ago

I got this as well when setting "bundledPackages": ["connect"], in Vite. Using api-extractor version 7.18.5. Stacktrace refers to the same method (new line number since new version):

Even though they all crash in _getExportOfAstModule(), the underlying reasons could be somewhat different.

BTW for a little background, API Extractor is really designed for documenting professionally shipped TypeScript APIs and tracking their signatures across version upgrades to avoid compatibility breaks. By nature such APIs tend to be fairly well behaved, because the .d.ts files are generated by the TypeScript compiler, and developers will put design thought into their API and use systematic patterns that are intelligible when presented as an API reference website. By contrast, a package like* @types/connect is basically a kludge for a library that was designed for unchecked JavaScript, and whose maintainers care so little about TypeScript that they aren't even bothering to ship their own .d.ts files. If the API is very simple, sometimes the kludge can be well behaved. But more often than not, describing those unchecked JavaScript constructs may require obscure TypeScript language features. Also, an @types package may use deprecated syntax or actually invalid syntax simply because it was authored by well-meaning people from the community who are merely casual contributors. For this reason, API Extractor is normally pretty forgiving when handling external declarations. However when you do "bundledPackages": ["connect"], this instructs API Extractor to merge them into the exported API for your library, and then all those little glitches can cause trouble for the analysis.

\*I emphasized ***"a package like"*** because I'm not familiar with `@types/connect` and didn't mean to imply anything about that project specifically.

octogonz commented 3 years ago

I'm not sure if that code should be updated though? It seems like a valid thing to do and am not sure how I'd change the actual senchalabs/connect package

Before we get into that, it's worth asking WHY are you trying to bundle someone else's declarations into your own library's .d.ts rollup? That is what "bundledPackages" does.

benmccann commented 3 years ago

I updated all of the Vite code to do import connect = require('connect') and api-extractor is now able to run. However, the bundlePackages config seems to be having no effect. When I open up the index.d.ts file in the dist directory I see import connect = require('connect'); instead of the code being included

As to why I'm trying to use bundledPackages - this is not my code. It's an open source library and I'm trying to make it less terrible than what they are doing now in a way that they will accept since they are for some reason allergic to dependencies.

octogonz commented 3 years ago

Would you be able to share a GitHub branch with your progress so far?

benmccann commented 3 years ago

Absolutely! Code is here: https://github.com/benmccann/vite/tree/connect-types

You can run api-extractor with:

yarn install
cd packages/vite
yarn build-types

thanks so much for the help!

octogonz commented 3 years ago

Super helpful, thank you!

Looking at your exported API, the only references to the connect package seem to be here:

packages/vite/src/node/server/index.ts

export declare interface ViteDevServer {
    /**
     * The resolved vite config object
     */
    config: ResolvedConfig;
    /**
     * A connect app instance.
     * - Can be used to attach custom middlewares to the dev server.
     * - Can also be used as the handler function of a custom http server
     *   or as a middleware in any connect-style Node.js frameworks
     *
     * https://github.com/senchalabs/connect#use-middleware
     */
    middlewares: connect.Server; πŸ‘ˆπŸ‘ˆπŸ‘ˆ
    /**
     * @deprecated use `server.middlewares` instead
     */
    app: connect.Server; πŸ‘ˆπŸ‘ˆπŸ‘ˆ

And the structure of that object is relatively simple:

    export type ServerHandle = HandleFunction | http.Server;

    export class IncomingMessage extends http.IncomingMessage {
        originalUrl?: http.IncomingMessage["url"] | undefined;
    }

    type NextFunction = (err?: any) => void;

    export type SimpleHandleFunction = (req: IncomingMessage, res: http.ServerResponse) => void;
    export type NextHandleFunction = (req: IncomingMessage, res: http.ServerResponse, next: NextFunction) => void;
    export type ErrorHandleFunction = (err: any, req: IncomingMessage, res: http.ServerResponse, next: NextFunction) => void;
    export type HandleFunction = SimpleHandleFunction | NextHandleFunction | ErrorHandleFunction;

    export interface ServerStackItem {
        route: string;
        handle: ServerHandle;
    }

    export interface Server extends NodeJS.EventEmitter {
        (req: http.IncomingMessage, res: http.ServerResponse, next?: Function): void;

        route: string;
        stack: ServerStackItem[];

        use(fn: NextHandleFunction): Server;
        use(fn: HandleFunction): Server;
        use(route: string, fn: NextHandleFunction): Server;
        use(route: string, fn: HandleFunction): Server;

        handle(req: http.IncomingMessage, res: http.ServerResponse, next: Function): void;

        listen(port: number, hostname?: string, backlog?: number, callback?: Function): http.Server;
        listen(port: number, hostname?: string, callback?: Function): http.Server;
        listen(path: string, callback?: Function): http.Server;
        listen(handle: any, listeningListener?: Function): http.Server;
    }

So rather than trying to embed the entire @types/connect in your API signature, perhaps you could simplify the declaration of connect.Server somehow? For example, declare it as any, or as a condensed equivalent of the above definitions?

Otherwise, if we really need the full @types/connect, I think I would approach it by trying to transform namespace createServer into something that is not exported using export = createServer. (Or even contribute a DefinitelyTyped PR to add a secondary export for those APIs -- they are 100% export type so this would not imply any changes to the underling connect package.)

benmccann commented 3 years ago

declare it as any, or as a condensed equivalent of the above definitions?

This doesn't sound like a great option to me because then I can't utilize in my consuming package

I think I would approach it by trying to transform namespace createServer into something that is not exported using export = createServer

I'd have to convince the connect authors to change their code. I can't just change the types because they must conform to the code. The connect package is downloaded 4.3m times / week, so I'm guessing they're probably not eager to introduce a breaking change to the way their package is exported. I'm guessing they'll just say that my tooling should support the Node.js spec

Or even contribute a DefinitelyTyped PR to add a secondary export for those APIs -- they are 100% export type so this would not imply any changes to the underling connect package.

Thanks for the idea. Unfortunately, I just tried this and I don't think it will work. It results in an error in the .d.ts file: An export assignment cannot be used in a module with other exported elements.

davidmurdoch commented 2 years ago

I believe the Emittery package also causes this error. Here is its d.ts: https://github.com/sindresorhus/emittery/blob/de3d78d67f9e099beff6384d1a59213c801562b6/index.d.ts#L597

PeachScript commented 2 years ago

@octogonz hi buddy, does the api-extractor plan to support export = syntax? A lot of packages within @types using export = syntax rather than export default, it's unrealistic to rewrite them all.

Otherwise, export = is incompatible with export default, but how does the TypeScript compiler process them, is it possible to pre-transform export = to export xx losslessly for api-extractor?

foxriver76 commented 1 year ago

Any news on this? Having the same issue when using types from winston https://github.com/winstonjs/winston/blob/23cb80c88db69af5205419e6cd77da9af6faa15c/index.d.ts#L219