microsoft / rushstack

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

[api-extractor] improve the .d.ts rollup to handle augmented types #1709

Open yangmingshan opened 4 years ago

yangmingshan commented 4 years ago

This issue proposes to improve API Extractor's analysis to handle augmented types, as illustrated in this repro from @manrueda:

https://github.com/manrueda/api-extractor-module-augmentation

In TypeScript, types can get "augmented" when the original declaration is extended locally in a file, via declaration merging. For example, we might have an entry point like this:

index.ts

// imports an interface declaration
import { Extras } from './core';

// adds more members to the interface, but only from the perspective of this file
import './plugin';

// exports the augmented result
export { Extras }

Note: The original issue description involved global variables, but we have narrowed the scope to only consider augmentation of normal exports. Details in this comment.

Original description involving global augmentation **Is this a feature or a bug?** - [ ] Feature - [x] Bug **Please describe the actual behavior.** .d.ts rollup generation will remove global type extension. **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.** Source code: ```ts declare global { namespace jest { interface Matchers { toHaveBeenWarned(): R toHaveBeenWarnedLast(): R toHaveBeenWarnedTimes(n: number): R } } } export interface User { name: string } ``` Generated code: ```ts export declare interface User { name: string } export { } ``` **What is the expected behavior?** Generated code should keep global type extension. **If this is a bug, please provide the tool version, Node.js version, and OS.** * **Tool:** api-extractor * **Tool Version:** 7.7.6 * **Node Version:** 13.7.0 * **Is this a LTS version?** No * **Have you tested on a LTS version?** No * **OS:** macOS Catalina 10.15.2
manrueda commented 4 years ago

Hi! We are having the exact same problem. We heavily use global types extensions and this is a blocker to implement api-extractor. Will this be fixed? If there is not enough band-width, can I get some guidance of how could I contribute a solution.

Thanks!

manrueda commented 4 years ago

Following the Module Augmentation pattern does not work either.

We use this patter a lot for plugin based logic where a plugin can extend a centralized interfaced to expose optional options for a common function. In this case for any extra options required by the plugin in the file plugin.ts only is required to augment the interface Extras. This reduce the requirement of implementing wrapper function on top of the core functionality and have strong types.

I tried to go down into the api-extractor source code and try to work out a PR but the code based is big and I would need some guidance on where will be the best place to detect the declare module symbols and surface those to the root.

Example:

// modules ./core.ts
export interface Extras {}

export function foo(options: Extras) {
// ...
}
// modules ./plugin.ts
declare module './core' {
  export interface Extras {
    someCoolExtra?: string;
  }
}
// modules ./index.ts
export * from './core';
export * from './plugin';
octogonz commented 4 years ago

Could you provide a more complete example? The above code doesn't seem to compile for me.

// TS2436: Ambient module declaration cannot specify relative module name
declare module './core' {

// TS2306: File 'plugin.ts' is not a module
export * from './plugin';
manrueda commented 4 years ago

Sorry about the oversimplified example. Here is a working repo example:

https://github.com/manrueda/api-extractor-module-augmentation

octogonz commented 4 years ago

Thanks @manrueda , this was super helpful! I added some fixes to the repro in PR https://github.com/manrueda/api-extractor-module-augmentation/pull/1 .

For your example, it seems like we want the .d.ts rollup to look like this:

export declare interface Extras {
    coreProp?: boolean;
}
export declare interface Extras {
    pluginProp?: boolean;
}

or even better, like this:

export declare interface Extras {
    coreProp?: boolean;
    pluginProp?: boolean;
}

Today, API Extractor's analyzer starts from the Extras export in index.ts:

export { foo, Extras }

...and follows aliases to get to the "followedSymbol" in core.ts:

export declare interface Extras {
    coreProp?: boolean;
}

It then essentially copy+pastes that declaration into the .d.ts rollup file. Suppose hypothetically that your symbol had multiple "merged declarations" in that file, like this:

core.ts

export declare interface Extras {
    coreProp?: boolean;
}
export declare interface Extras {
    moreStuff?: boolean;
}

In that case, the analyzer would make a single AstSymbol (i.e. ts.Symbol in the compiler engine) with two associated AstDeclaration (i.e. ts.Declaration) objects, corresponding to these two interfaces. Thus the .d.ts rollup would correctly capture both of them.

But with augmented types, there's a new challenge: The type of a symbol can be different from the perspective of different files. In your example, the followedSymbol.declarations has only one declaration in core.ts. But from the perspective of index.ts there is now a second declaration coming from plugin.ts.

Playing around with this in API Extactor, I could not find any ts.Symbol with two declarations. Instead, it seems that we can call ts.TypeChecker.getDeclaredTypeOfSymbol(exportSymbol) which returns a ts.Type object that will have both coreProp and pluginProp in its ts.Type.declaredProperties list. But for this to work, we need to use exportSymbol from index.ts. If we use followedSymbol from core.ts then the resulting ts.Type will only have one property.

This raises a concern about uniqueness -- if the same symbol appears in different contexts, which augmentation should API Extractor choose? For exported symbols, I think the answer is use the entry point file (index.ts). For forgotten exports (or intentionally unexported types), I'm not sure there is a well-defined answer, and maybe it would be better to ignore augmentation for that case.

A second challenge is how to make the .d.ts rollup. The compiler engine seems to treat ts.Type as a purely semantic object: If it's an interface, ts.Type can tell you its properties and base interfaces. If it's a function, ts.Type can tell you its call signatures, etc. But if we want to find the original interface declarations from your .d.ts outputs, apparently the only way is to examine each member declaration, then walk upwards in the AST to find the interface parent. We could instead try to use the compiler engine to emit a new merged declaration, but API Extractor generally avoids that, preferring to excerpt existing .d.ts content. (This approach better preserves the original API form, and it probably eliminates a lot of code that would otherwise be involved with emitting every kind of TypeScript type expression.)

We could just say that augmented interface is the only variation that API Extractor will support. But there are many other kinds of declaration merging. For example, merging like this is valid:

// core.ts
export interface Extras {
  coreProp?: boolean;
}

// plugin.ts
declare module './core' {
  export function Extras(): void;
}

And so is this:

// core.ts
export enum Extras {
  coreProp = 1
}

// plugin.ts
declare module './core' {
  export namespace Extras { }
}

Thinking about this... I feel like we need to find/invent some compiler black magic that lets us retrieve the original ts.Declaration objects that were used to make the augmented ts.Type. If someone can figure that out that part, then šŸ¤” I'm somewhat optimistic that we can make AstDeclaration objects for the augmented interfaces and have a pretty sound algorithm that handles every kind of type.

octogonz commented 4 years ago

BTW looking back at @yangmingshan 's original issue description, it seems that his request involed globally-scoped augmented declarations, which is a fairly different problem. Today API Extractor doesn't even support non-augmented globals. (Global variables are frankly a not a great way to define an API, so there hasn't been a lot of interest in doing the work.)

In https://github.com/vuejs/vue-next/issues/652#issuecomment-577207973 it looks like they realized that jest.Matchers wasn't supposed to be in the .d.ts rollup and closed their issue.

Thus I'm going to repurpose this issue to focus on @manrueda 's repro instead.

manrueda commented 4 years ago

Thanks @octogonz! I was using global augmented types to have my extensibility feature but I am changing to do module based to have a cleaner API.

I expand the example repo with another example that I want to have that is extending a type from an NPM package. https://github.com/manrueda/api-extractor-module-augmentation/commit/e25c974f1b9d0cffaaee8080164c2a9073501cc7

Not sure if that makes thing way more complicated or not. I used an interface from @microsoft/api-extractor just for example purpuses.

octogonz commented 4 years ago

Not sure if that makes thing way more complicated or not.

It does. šŸ˜ But I had totally not thought about that possibility. It is an enlightening case to consider, even if we don't support it initially.

I tried to go down into the api-extractor source code and try to work out a PR but the code based is big and I would need some guidance on where will be the best place to detect the declare module symbols and surface those to the root.

BTW if someone wants to pitch in, it would be super helpful for someone to figure out how to enumerate the ts.Declaration objects that the compiler engine merged when creating its ts.Type object. (I'm reachable on Gitter if more info is needed.)

manrueda commented 4 years ago

Something that I just remember (and why I end up in the global augmentation issue) it's that we have a less important case where we extend the TypeScript build it HTMLElementTagNameMap interface to declare custom elements and get all the nice types integrations in querySelector, createElement, etc.

cdauth commented 12 months ago

Is there any progress on this? Are there any known workarounds?

ocavue commented 12 months ago

Are there any known workarounds?

I just write a simple script to read declare ... from source .ts files, and append these code to the .d.ts.

// api-extractor doesn't support `declare global`. This function is a workaround for it.
// See also https://github.com/microsoft/rushstack/issues/1709
async function copyDeclare(pkg: Package) {
  const sourceFiles = await glob(['**/*.ts', '**/*.tsx'], {
    cwd: path.join(pkg.dir, 'src'),
    absolute: true,
  });

  const chunks: string[] = [];

  for (const filePath of sourceFiles) {
    const content = await readFile(filePath, { encoding: 'utf-8' });
    const lines = content.split('\n');
    const startIndex = lines.indexOf('declare global {');

    if (startIndex === -1) {
      continue;
    }

    chunks.push(lines.slice(startIndex).join('\n'));
  }

  if (chunks.length === 0) {
    return;
  }

  const code = `\n${chunks.join('\n\n')}`;

  const destFiles = await glob(['_tsup-dts-rollup*'], {
    cwd: path.join(pkg.dir, 'dist'),
    absolute: true,
  });

  for (const filePath of destFiles) {
    const content = await readFile(filePath, { encoding: 'utf-8' });
    await writeFile(filePath, content + code);
  }
}
cdauth commented 10 months ago

In case this is helpful to anyone. I'm using api-extractor through vite-plugin-dts with rollupTypes: true.

As a workaround for this issue, I moved all my module augmentations to a single file (src/type-extensions.ts) and simply manually append its content to the output bundle using the following configuration:

import dtsPlugin from "vite-plugin-dts";
import { appendFile, readFile } from "fs/promises";

export default defineConfig({
    plugins: [
        dtsPlugin({
            rollupTypes: true,
            async afterBuild() {
                const filterFile = await readFile("./src/type-extensions.ts");
                await appendFile("./dist/facilmap-leaflet.d.ts", filterFile);
            },
        }),
WickyNilliams commented 6 months ago

I followed @cdauth's suggestion, which has been a helpful workaround. however, i settled on a d.ts file rather than a .ts file for the module augmentations.

I am augmenting HTMLElementTagNameMap with some custom elements i have written in another file in my repo. if i use .ts then i need to import the types from the modules to silence typescript. but that means the file can't be blindly appended as above because the import paths will be wrong. but a d.ts file you won't get any errors.

so i ended up with something like this:

// globals.d.ts

declare global {
  interface HTMLElementTagNameMap {
    "my-element": MyElement;
    "another-element": AnotherElement;
  }
}

and otherwise the vite config is as described. thanks for that suggestion, it really helped my situation