dts-bundle-generator icon indicating copy to clipboard operation
dts-bundle-generator copied to clipboard

Star re-exports (`import * as NS` / `export * as NS`) aren't wrapped with a `NS` name

Open schleumer opened this issue 4 years ago • 15 comments

Bug report

I've used dts-bundle-generator to generate bundle of .d.ts for use on Monaco Editor and some wierd Typescript compiler that runs on GraalJS that i've made. I have 2 globals which are actually aliases to npm modules, which are Ramda and Cheerio, they worked great on some old .d.ts files, but when i've updated those libraries to its recent version, the code generation didn't work.

Input code

import * as R from "ramda";
import * as cheerio from "cheerio";

export { R, cheerio }

Expected output

Something around this: https://gist.github.com/schleumer/3c0c7d468e050b2163760e7e4392d5cd, of course, those libs changed a lot and may be slightly different

Actual output

// Generated by dts-bundle-generator v5.4.0



export {
	 as R,
};

Additional context

I've made a custom script, something like:

const generated = generator.generateDtsBundle([
    {
        filePath: 'src/libs.d.ts'
    }
], {
  preferredConfigPath: './tsconfig.json',
  followSymlinks: true,
});

Also, with internal files, dts-bundle-generator works great, my only issue is with importing Ramda and Cheerio, don't know if there's something new that i've lost.

Thanks for the awesome library :)

schleumer avatar Aug 24 '20 01:08 schleumer

Probably it was broken in 5.4.0 in #132. As workaround you can try to use

export * as R from "ramda"
export * as cheerio from "cheerio";

timocov avatar Aug 24 '20 06:08 timocov

@timocov, i've uploaded a repository with some tests i've made, i also added an example with the export star, which didn't worked :/

schleumer avatar Aug 24 '20 16:08 schleumer

Also, with internal files, dts-bundle-generator works great, my only issue is with importing Ramda and Cheerio, don't know if there's something new that i've lost.

i've uploaded a repository with some tests i've made, i also added an example with the export star, which didn't worked :/

@schleumer Thanks for the repo with tests! Is it intended that you'd like to inline these libraries into a bundle, right?

timocov avatar Sep 27 '20 08:09 timocov

Just wonder what output here could be... Wrapped ramda with namespace R {} and cheerio with namespace cheerio {}? What if you'll refer to that libraries with import {} from 'ramda' instead of import * as R from 'ramda' (named imports instead)? Should it be inlined twice? @schleumer can you provide example(s) where and when it's helpful to export others packages in this way and inline them rather them just re-export please?

timocov avatar Sep 27 '20 10:09 timocov

@timocov I read through this issue and I think that this issue (#134) and the issues #139 and #146 describe different problems.

From what I understand, this issue is about that namespaced imports to external modules. However, #139 and #146 are about namespaced imports to local files. This bundler has to generate different code in those situations. While external modules should be referenced, local files have to inlined.

I do not know whether #134, #139, and #146 are all caused by the same underlying issue but they do require different solutions.

To answer your questions as to what the correct output should be:

This issue

The input code of #134:

import * as R from "ramda";
import * as cheerio from "cheerio";

export { R, cheerio }

should have the output:

import * as R from "ramda";
import * as cheerio from "cheerio";

export { R, cheerio }

This is what I would say is correct. This is a valid .dts file and it behaves exactly like the input code.

The other issues

The input code of #139 (slightly modified to make it more clear):

// index.ts
export type A = number;

export * as Bar from "./bar";

// ./bar.ts
export type B = number;

should have the output:

export declare type A = number;

export declare namespace Bar {
    export declare type B = number;
};

The input code of #146 (credit to @bradzacher):

// file1
export interface Foo {}
export interface Bar {}

// file2
export * as Test from './file1';

should have the output (slightly modified by me):

export declare namespace Test {
  export declare interface Foo {}
  export declare interface Bar {}
}

RunDevelopment avatar Apr 08 '21 16:04 RunDevelopment

Added a failing test for this #154, trying to fix the actual issue too but having some problems understanding TS AST and where to put the fix 😓

alexandernanberg avatar Apr 09 '21 16:04 alexandernanberg

@alexandernanberg If you have trouble understanding TS AST, then AST explorer might be helpful. Unfortunately, I can't help you with the fix.

RunDevelopment avatar Apr 09 '21 16:04 RunDevelopment

The input code of #134: should have the output:

@RunDevelopment did you see that https://github.com/schleumer/dts-bundle-generator-tests/blob/e4a16a709215765dad6ba29c409336d428e24e01/broken-latest-ramda-and-cheerio/bundle.js#L17 these libraries are expected to be inlined, so we might treat them like a "internal" ones.

The input code of #139 (slightly modified to make it more clear): should have the output:

This is pretty easy case, but the problem is to handle something like this:

// index.ts
import { Foo } from './foo';

export type A = number | Foo;
export * as Bar from "./bar";

// bar.ts
export type B = number;

// foo.ts
import { B } from './bar';
export type Foo = B | string;

As you can see here we have 2 types of accessing to types from bar.ts module: via * as Bar and { B }. So to handle this we have to do renaming which is tricky to implement at the moment (not sure whether this is because of lacks of API or my non-deep knowledge of it). If you have any idea how it could be done, I'll be happy to chat about that/implement it.

timocov avatar Apr 09 '21 20:04 timocov

did you see that [...] these libraries are expected to be inlined, so we might treat them like a "internal" ones.

I didn't. My bad. In this case, all three issues are equivalent.

the problem is to handle something like this

@timocov I see that your example is a little more complex but it is still solvable.

type Foo = Bar.B | string;
export type A = number | Foo;
export declare namespace Bar {
	type B = number;
};

That being said, your example most certainly isn't easy to solve in general. The main problem is that the dependencies form a DAG (let's hope it's acyclic) which is much harder to handle than a tree. I don't know whether this will affect how hard/easy the problem is to solve, but maybe it's enough to only support dependency trees for namespaces for now?

So to handle this we have to do renaming

Does renaming include adding namespace prefixes? From what I see, the bundler only needs to add namespace prefixes.

If you have any idea how it could be done,

"it" - the renaming or the namespace creation?

RunDevelopment avatar Apr 09 '21 22:04 RunDevelopment

I don't know whether this will affect how hard/easy the problem is to solve, but maybe it's enough to only support dependency trees for namespaces for now?

Probably in this case we can add another limitation to the list in readme (if you use * as NS for some module, you have to use it everywhere in your code so it will be 100% safe, otherwise it might produce wrong code). And it's better for you if you don't have names with the same name, but if you do, it might produce correct code in terms of compilation but wrong in terms of logic:

// index.ts
import { Foo } from './foo';

export type A = number | Foo;
export type B = string;
export * as Bar from "./bar";

// bar.ts
export type B = number;

// foo.ts
import { B } from './bar';
export type Foo = B | string;

In this case, if we just "wrap" a whole module into namespace we'll get the following:

export type Foo = B | string;
export type A = number | Foo;
export type B = string;
export namespace Bar {
    export type B = number;
}

which will compile, but has completely wrong meaning because Foo uses different type B (it should be Bar.B instead of B).

Another thing we have to worry about is to do "tree shaking" inside the namespace, so we have remove all unused types from that "artificial" namespaces (currently the tool doesn't have such feature for "natural" namespaces though, but since we're talking about generated ones I think we need to worry about that).

Does renaming include adding namespace prefixes? From what I see, the bundler only needs to add namespace prefixes.

Yes. If we're talking about handling all cases, we need to think about "true-renaming" with following the dependencies tree.

"it" - the renaming or the namespace creation?

Oops, sorry for confusing. Renaming for sure.

timocov avatar Apr 10 '21 08:04 timocov

it might produce wrong code

Can't we fail instead of producing wrong code? People (e.g. I) rely on this being correct.

Renaming

Frankly, I would love to do a deep dive into this. This seems like an interesting problem but I don't have the time right now. If this is still a problem in a few weeks from now, I'll do a PR.

RunDevelopment avatar Apr 10 '21 11:04 RunDevelopment

Can't we fail instead of producing wrong code?

If we can detect this then yes, I'd prefer to handle this and report an error for that for sure.

timocov avatar Apr 10 '21 18:04 timocov

Hello @timocov does this conditionals help solve:

  • Put in root namespace if no name collision
  • Pack into a namespace otherwise
  • The name of namespace can be the name it's exported with
  • If it is not exported or has collision, pick a random namespace name

wolfram77 avatar Apr 22 '22 18:04 wolfram77

@wolfram77 can you provide examples for each please? I'm not sure that I understood your point correctly, I'm sorry.

timocov avatar Apr 24 '22 10:04 timocov

@timocov Please check the examples taken from above.


Example 1

#134 as given by @RunDevelopment

Original input:

// bar.ts
export type B = number;

// index.ts
export type A = number;
export * as Bar from "./bar";

This can be rewritten by wrapping bar.ts into a unique namespace as:

declare namespace ts_bar {
  export declare type B = number;
}

export declare type A = number;
export ts_bar as Bar;

Since namespace ts_bar is used only once (one custom name), this can be simplified as:

export declare type A = number;
export declare namespace Bar {
  export declare type B = number;
}

Example 2

#139 as given by @RunDevelopment

Original input:

// file1.ts
export interface Foo {}
export interface Bar {}

// file2.ts
export * as Test from './file1';

Can be rewritten as:

declare namespace ts_file1 {
  export declare interface Foo {}
  export declare interface Bar {}
}

export ts_file1 as Test;

Which can then be simplified as:

export declare namespace Test {
  export declare interface Foo {}
  export declare interface Bar {}
}

Since there are no other usages of namespace ts_file1.


Example 3

As given by @timocov

Original input:

// bar.ts
export type B = number;

// foo.ts
import { B } from './bar';
export type Foo = B | string;

// index.ts
import { Foo } from './foo';
export type A = number | Foo;
export * as Bar from "./bar";

Can be rewritten as (wrapping into namespace):

declare namespace ts_bar {
  export declare type B = number;
}

declare namespace ts_foo {
  export declare type Foo = ts_bar.B | string;
}

export declare type A = number | ts_foo.Foo;
export ts_bar as Bar;

Which can then be simplified as:

export declare namespace Bar {
  export declare type B = number;
}

declare namespace ts_foo {
  export declare type Foo = Bar.B | string;
}

export declare type A = number | ts_foo.Foo;

Now only ts_foo.Foo is used and there is no collision in root namespace, so:

export declare namespace Bar {
  export declare type B = number;
}

declare type Foo = Bar.B | string;

export declare type A = number | Foo;

Example 4

As given by @timocov

Original input:

// bar.ts
export type B = number;

// foo.ts
import { B } from './bar';
export type Foo = B | string;

// index.ts
import { Foo } from './foo';
export type A = number | Foo;
export type B = string;
export * as Bar from "./bar";

Can be rewritten as:

declare namespace ts_bar {
  export declare type B = number;
}

declare namespace ts_foo {
  export declare type Foo = ts_bar.B | string;
}

export declare type A = number | ts_foo.Foo;
export declare type B = string;
export ts_bar as Bar;

Which can then be simplified as:

export declare namespace Bar {
  export declare type B = number;
}

declare namespace ts_foo {
  export declare type Foo = Bar.B | string;
}

export declare type A = number | ts_foo.Foo;
export declare type B = string;

Which can then be re-simplified as (ts_foo.Foo):

export declare namespace Bar {
  export declare type B = number;
}

declare type Foo = Bar.B | string;

export declare type A = number | Foo;
export declare type B = string;

Example 5

#134 as given by @RunDevelopment

import * as R from "ramda";
import * as cheerio from "cheerio";

export { R, cheerio }

Can be simplified to:

export * as R from "ramda";
export * as cheerio from "cheerio";

I would like to fix the conditionals:

  • Pack each imported file into a random namespace (in root namespace)
  • Rename symbols accordingly (namespace.symbol)
  • If a namespace is used only once (or same name), use the custom namespace name
  • If a member of namespace has no collision in root namespace, put in root

At each step, symbols need to be renamed appropriately.

wolfram77 avatar Apr 24 '22 12:04 wolfram77

@timocov

Idea from @wolfram77's seems like feasible.

For example TS AST representation of following is:

// CODE
export * as R from "ramda";

// AST (node.forEachChild)
SourceFile
    ExportDeclaration
        NamespaceExport
          Identifier
        StringLiteral
EndOfFileToken

// AST (node.getChildren())
SourceFile
    SyntaxList
        ExportDeclaration
            ExportKeyword
            NamespaceExport
                AsteriskToken
                AsKeyword
                Identifier
            FromKeyword
            StringLiteral
            SemicolonToken
EndOfFileToken

We could get identifiers from typescript AST, asterixes (if needed at all since we know it's namespace export) etc. And potentially continue using tree representation (just dependency trees) instead of graph as @RunDevelopment suggested

I have same problem in my project. A named star (re)export is not included. As it's NamespaceExport in TS AST we could be able to process it according to @wolfram77's rules

duki994 avatar Dec 01 '22 10:12 duki994

Is there someone on this actually?

yovanoc avatar Mar 09 '23 13:03 yovanoc

Hi everybody,

I'm really sorry for not replying in this thread for a long time.

I do really think that this and name collision problems (#184, #116 and #130) are the most complex problems in this tool (especially renaming one as it requires deep diving into typescript's AST manipulation which I don't have much experience with yet).

At the same time, I do understand that these 2 problems are extremely important (and always have been since very first published version) and someone would prefer to use different tools (see #68) instead that might not have these problems.

If I'm not mistaken, to transform AST in the form above it is not that simple as just "replace a node with a new node with different name", it requires to traverse every single node in the project and check if that renamed (or converted) node is referenced and then replace it accordingly. From my perspective, this is huge amount of work, effort and time which I am not always have, unfortunately. But I could be wrong and would be happy to be proven wrong if someone has understanding and vision how to implement it and can spend their time to do it.


Another idea

Meanwhile, I think I might have an idea how to solve the problem without implementing renaming mechanism, but it would have another limitation. The idea is the following:

  • we keep bundle generation the same as it is right now (i.e. all nodes are added as they are now, without any renaming), apart from export keyword (details below)
  • if in the entry point we have export * as NS, we create and add a namespace NS with re-exports of every node that is supposed to be re-exported from that namespace (see example below)
  • a node that is supposed to be exported via namespace only (i.e. no direct export) will not have export keyword even if it can be exported as it is for interfaces/types right now

Limitations/open questions

  • names collision is still here and you will have to keep names unique within a bundle
  • additional to the previous one - as export * as NS creates a namespace NS it means that a bundle shouldn't have other nodes with NS name
  • as soon as no renaming is done you still should not use import * as ns for local modules where it is supposed to be exported
  • not sure what to do with export keyword for nodes that are both exported via namespace AND via other types indirectly

Example

I'll take one of the examples above, don't think there is a big difference between them for this solution, but feel free to post your ideas/examples in the replies.

// bar.ts
export class Bar {}

// foo.ts
import { Bar } from './bar';
export type Foo = Bar | string;

// index.ts
import { Foo } from './foo';
export type A = number | Foo;
export type B = string;
export * as Bar1 from "./bar";
export * as Bar2 from "./bar";
export * as Foo from "./foo";

as the result it would be something like this:

declare class Bar {}
declare type Foo = Bar | string;

export declare type A = number | Foo;
export declare type B = string;

export declare namespace Bar1 {
    export { Bar };
}

export declare namespace Bar2 {
    export { Bar };
}

export declare namespace Foo {
    export { Foo };
}

What do you all think about it? For me, it feels quite feasible in the matter of implementation and effort and it kinda solves the problem (partially tho). But I'm afraid that limitations might not satisfy these who really need this feature in full power, so I'll be waiting for feedback.

timocov avatar Mar 30 '23 23:03 timocov

Hello @timocov I don't see any limitation in the given example. I currently use re-exports as follows:

/// number.ts
export function constrain(x: number, min: number, max: number): number;
...

/// math.ts
export {constrain} from "./number"
export function magnitude(xs: number[]): number;
$ dts-bundle-generator -o index.d.ts math.ts
/// index.d.ts
// Generated by dts-bundle-generator v8.0.0

export declare function magnitude(xs: number[]): number;

As you see it did not export constrain(). I have not read typescript AST, so cant comment it might be solved. Thanks for detailed comment and this tool. An aside, if you are working on high-performance TypeScript/Web project, i am interested.

wolfram77 avatar Apr 01 '23 10:04 wolfram77

@wolfram77 the given example seems not related to the problem as you don't use export * as NS syntax. Also, can you create ta repro for your problem? I just tried to replicate your example in tests and it generates the correct output:

export declare function constrain(x: number, min: number, max: number): number;
export declare function magnitude(xs: number[]): number;

export {};

Anyway, if you're able to reproduce the problem with the given example, please create another issue so we won't spam this long thread with off-topic messages. Thanks!

timocov avatar Apr 01 '23 10:04 timocov

@timocov It seems i forgot to use --external-inlines. number.ts was in another package. It works.

wolfram77 avatar Apr 01 '23 10:04 wolfram77

Hey all.

I don't know if it will be of any help, but I wanted to drop a quick note on how I'd handle this if I were building from scratch.

Quick caveats:

  • I went through the code a bit while implementing a workaround but I did not go through it enough to understand the whole flow.
  • I recognize that there are likely edge cases I'm not fully thinking of immediately

So there is my disclaimer.

With that said, in case it helps...

Walking Phase

  1. Compile DTS first
  2. Walk statements and resolve out types, following files that are not external
  3. Maintain an array of metadata

something like...

interface TypeData {
  srcName: string
  dstName: string
  declaration: DeclarationNode
  symbol: Symbol
  sourceFile: SourceFile
  children?: TypeData[]
  isRootExported: boolean
}

Notes:

  • children is populated with data from within NamespaceExport / NamespaceImport (ie import / export * as XX from '...';)
  • Name collisions with non-root exported types get auto-renamed
    • Like with a js compiler, simply append _<number> ie. type MyType = string = type MyType_1 = string
    • This is safe because nothing outside of the module uses it
  • Name collisions with root exported types either (based on config setting):
    1. warn + rename - apply the same renaming scheme as detailed above
    2. error - throw an error
    3. handler - pass to handler fn supplied in config to allow the user to resolve

Writing Phase

  1. Iterate array of metadata and create new Statement[]
  2. Create new SourceFile
  3. Print / error check

Notes:

  • When writing a module (with children):
    • Wrap in namespace
    • Ensure all written are safe for ambient context (no declare keywords)
    • Any Symbol that exists in the main list of TypeData should be able to be simply re-exported
    • Those that are new Symbols would be written as usual

Further Notes

FWIW - I think trying to merge type declarations into unions, etc. would not be expected behaviour and would lead to issues. If I had two files with a type having the same name, I'd definitely prefer they not be merged. Instead, getting an error or a rename with warning is good.

Having the option to handle via middleware would be great.

I also think that having more logical separation between the walking and writing phases may help simplify things, but again, I've not fully familiarized myself with your code.

In any event, thank you for the useful library! Hopefully this will come in handy, but if not, I look forward to seeing your eventual solution.

  • R

nonara avatar Jul 18 '23 00:07 nonara