benmerckx / genes

Generates split ES6 modules and Typescript definitions from Haxe modules.
43 stars 8 forks source link

Test name clash #35

Closed kevinresol closed 3 years ago

kevinresol commented 3 years ago

This PR adds tests for name clashes for same-named types in different packages. And the dynamic import case failed.

kevinresol commented 3 years ago

I tried to make Genes.ignore take fully-qualified type names and that successfully prevent generating the static import statements for the import-as types. But then the generated body does not distinguish the two types, i.e. both new FooClass() and new BarClass() is generated as new MyClass()

kevinresol commented 3 years ago

Also just realized that abstract needs special handling. i.e. load both the base type and the abstract _Impl_ type. Maybe I will attempt that on another PR.

kevinresol commented 3 years ago

Actually now I think that genes should auto detect all the used types in the body without relying on the function arguments. Because inlining may bring types into context which will then not be ignored.

benmerckx commented 3 years ago

What do you suggest the api to look like? I'm not sure I follow exactly.

kevinresol commented 3 years ago

With current API:

class A {
    static inline function run()
        return [A.foo(), B.bar()];

      static function foo()
        return 'foo';

}
class B {
    static function bar()
        return 'bar';
}

dynamicImport(A -> A.run());

Since A.run() will be generated as [A.foo(), B.bar()] because of inlining, B will be statically imported in the generated js. (Because only A is ignored)

In fact users can manually include B in the import list by doing (A, B) -> A.run() but that means users have to handle inlining themselves, which is hard to use.

Instead I propose the API to be like dynamicImport(() -> A.run()); then genes will inspect the generated expr, collect all the types that are directly referenced, and do its import magics. In this case both A and B will be dynamically imported.

The generated code should look something like:

Promise.all([import('./A'), import('./B')]).then(modules => {
    var A = modules[0].A;
    var B = modules[1].B;
    return [A.foo(), B.bar()];
})
benmerckx commented 3 years ago

Wouldn't that mean we'd need to "know" whether types used in that expression are used outside of that scope? I'm not sure if there's a way to track that reliably.

import my.module.A;
import my.module.B;

final useBRightHere = new B();

final deferA = dynamicImport(() -> new A(new B()));
kevinresol commented 3 years ago

I think it would be fine if Genes.ignore does nothing to already statically imported modules. That is, in your example, B will be both statically and dynamically imported. I'm not exactly sure what will happen if the same module is both statically and dynamically imported. But I guess it should be clever enough to reuse a statically imported module when it is later dynamically loaded.

benmerckx commented 3 years ago

Maybe we could put that behaviour under a different method 'dynamicImportAll' or something? Since I like the option of being explicit where it makes sense.

But other than that discussion, is this good to merge?

kevinresol commented 3 years ago

Maybe we could put that behaviour under a different method 'dynamicImportAll' or something? Since I like the option of being explicit where it makes sense.

Okay

But other than that discussion, is this good to merge?

Yes I think so

kevinresol commented 3 years ago

Keeping track the remaining TODOs here: https://github.com/benmerckx/genes/issues/39