Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
schleumer opened this issue Aug 24, 2020 · 22 comments · Fixed by #281
Closed

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

schleumer opened this issue Aug 24, 2020 · 22 comments · Fixed by #281
Assignees
Labels
Milestone

Comments

@schleumer
Copy link

schleumer commented Aug 24, 2020

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 :)

@timocov timocov added the Bug label Aug 24, 2020
@timocov
Copy link
Owner

timocov commented Aug 24, 2020

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 timocov added this to the 5.5 milestone Aug 24, 2020
@schleumer
Copy link
Author

@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 :/

@timocov
Copy link
Owner

timocov commented Sep 27, 2020

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
Copy link
Owner

timocov commented Sep 27, 2020

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 timocov modified the milestones: 5.5, Future Nov 8, 2020
@timocov timocov changed the title Strange output for some libraries Star re-exports (import * as NS / export * as NS) aren't wrapped with a NS name Jan 28, 2021
@RunDevelopment
Copy link

@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 {}
}

@alexandernanberg
Copy link

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 😓

@RunDevelopment
Copy link

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

@timocov
Copy link
Owner

timocov commented Apr 9, 2021

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.

@RunDevelopment
Copy link

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?

@timocov
Copy link
Owner

timocov commented Apr 10, 2021

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.

@RunDevelopment
Copy link

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.

@timocov
Copy link
Owner

timocov commented Apr 10, 2021

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.

@wolfram77
Copy link

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

@timocov
Copy link
Owner

timocov commented Apr 24, 2022

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

@wolfram77
Copy link

@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.

@duki994
Copy link

duki994 commented Dec 1, 2022

@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

@yovanoc
Copy link

yovanoc commented Mar 9, 2023

Is there someone on this actually?

@timocov
Copy link
Owner

timocov commented Mar 30, 2023

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.

@wolfram77

This comment was marked as off-topic.

@timocov

This comment was marked as off-topic.

@wolfram77

This comment was marked as off-topic.

@nonara
Copy link

nonara commented Jul 18, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants