System or AMD module with outFile and dynamic import incorrect

[kitsonk]

TypeScript Version: 3.8.3

Search Terms:

dynamic import, system, outFile

Code

index.ts

import * as foo from "./mod1";

(async () => {
  const foo1 = await import("./mod1");
  console.log(foo, foo1);
})();

mod1.ts

export const foo = "foo";

$ tsc index.ts --outFile bundle.js --module system --target esnext

or

$ tsc index.ts --outFile bundle.js --module amd --target esnext

Expected behavior:

With System, it is expected that the dynamic import() statement would be converted to import("mod1") instead of staying as import("./mod1") since all modules are now registered with absolute specifiers instead of resolved specifiers. This is mostly because the compiler determines what the specifiers are named, and shouldn't presume relative resolution can be applied by the loader.

Actual behavior:

for System module

System.register("mod1", [], function (exports_1, context_1) {
    "use strict";
    var foo;
    var __moduleName = context_1 && context_1.id;
    return {
        setters: [],
        execute: function () {
            exports_1("foo", foo = "foo");
        }
    };
});
System.register("index", ["mod1"], function (exports_2, context_2) {
    "use strict";
    var foo;
    var __moduleName = context_2 && context_2.id;
    return {
        setters: [
            function (foo_1) {
                foo = foo_1;
            }
        ],
        execute: function () {
            (async () => {
                const foo1 = await context_2.import("./mod1");
                console.log(foo, foo1);
            })();
        }
    };
});

cc/ @guybedford

[guybedford]

Because dynamic import is based on expressions - custom expression remapping system is needed here which is very different to static module specifiers.

Typically I run an analysis parse over the ES source to compute a wildcard expression that reads a statement like:

const feature = 'listing';
import('./${feature}/${item}.js')

into a wildcard expression like "./listing/*.js". I then glob the file system to determine the possibilities and apply those as entry points in the build analysis. Then for bare specifier mappings applying that to the base where possible, otherwise relying on the alignment of the production import map with the original specifier in the import map generation process.

All of the above involves some quite high bandwidth connection between source analysis and the overall build process so I typically run this as a preparser.

Better integrating the above workflow into TypeScript analysis would of course be a huge win, it's just difficult to justify when very few people are quite there in getting on top of this conceptually.

[kitsonk]

@guybedford yes, I agree with the complexity of what could be passed in import(). Personal feeling though is that, I would accept that if TypeScript is able to resolve the import() to a symbol to a module that it uses for setting the type, that it should emit that symbols remapped name. There are a lot of situations where that would fall down, but it should fall down in line with TypeScript's ability to resolve the symbol.

[guybedford]

If the dynamic import is just a static string it could be a special option certainly to achieve what you want, but it is just the base case of what should really be a better engineered expression handler than that and so I don’t see it as the best solution for TypeScript to handle but that is not to say it can’t.

…

On Wed, Mar 18, 2020 at 01:33 Kitson Kelly ***@***.***> wrote: @guybedford <https://github.com/guybedford> yes, I agree with the complexity of what could be passed in import(). Personal feeling though is that, I would accept that if TypeScript is able to resolve the import() to a symbol to a module that it uses for setting the type, that it should emit that symbols remapped name. There are a lot of situations where that would fall down, but it should fall down in line with TypeScript's ability to resolve the symbol. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#37429 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESFSWL3HQWEAKW3ILUZS3RIACFDANCNFSM4LM5PBFA> .

[rbuckton]

Honestly, I'm not sure we're doing the right thing for single-file SystemJS or AMD output right now anyways:

• Named System.register is an extra feature, and is not enabled by default, but we have no way to message this as part of our build process.
• The above code registers index.ts using System.register("index", ...) (or define("index", ...)) which is not what you would want when distributing a library, and could cause conflicts if you were to want to import a package named foo but also had a foo.ts.

For most of our other module transformers we don't rewrite paths for import, and generally rely on "paths" mappings in tsconfig.json so that we understand imports the way they're written even if the actual file layout differs. What we probably should be doing here, if we are going to rewrite paths at all, is require something like a packageName compiler option for bundled output cases that would act as the base path for relative path resolution, and a packageMain option to indicate the package root, i.e.:

// tsc index.ts --outFile bundle.js --module system --target esnext --packageName foo --packageMain index.ts
System.register("foo/mod1", [], function (exports_1, context_1) {
    "use strict";
    var foo;
    var __moduleName = context_1 && context_1.id;
    return {
        setters: [],
        execute: function () {
            exports_1("foo", foo = "foo");
        }
    };
});
System.register("foo/index", ["foo/mod1"], function (exports_2, context_2) {
    "use strict";
    var foo;
    var __moduleName = context_2 && context_2.id;
    return {
        setters: [
            function (foo_1) {
                foo = foo_1;
            }
        ],
        execute: function () {
            (async () => {
                const foo1 = await context_2.import("foo/mod1");
                console.log(foo, foo1);
            })();
        }
    };
});
// NOTE: anonymous register that re-exports `--packageMain`
System.register(["foo/index"], function (exports_3, context_3) {
    "use strict";
    var index;
    var __moduleName = context_3 && context_3.id;
    function exportStar_1(m) {
        var exports = {};
        for (var n in m) {
            if (n !== "default") exports[n] = m[n];
        }
        exports_3(exports);
    }
    return {
        setters: [
            function (index_1) {
                index = index_1;
                exportStar_1(index_1);
            }
        ],
        execute: function () {
        }
    };
});

/cc @weswigham @DanielRosenwasser

[kitsonk]

While the specific use case that drove this issue is no longer needed personally, dynamic imports are clearly a "troublesome" situation for anyone else. Because with an outFile emit, I believe the SourceFile.moduleName is used in the emit for the System.register(name, [...], function () {}) and in the dependency array. For consistency, if the import(mid) symbol is resolved to another source file, for type checking purposes, it should be emitted as the source files moduleName.

On the other hand, for Deno, where we ended up is not touching dynamic imports at all for now, since they become such a complex problem, where does someone actually want to inline the import or not. I expect there are two main reasons that some wants to use dynamic import():

• Lazily load additional modules. Where the developer likely doesn't want the code in the bundle.
• Conditionally load something... like if condition is true at runtime, load a module or otherwise load a different module (e.g. loading polyfills). In that case the developer likely wants a bundle that has no further external dependencies.

So it almost feels like including or not including resolvable dynamic imports should be optional, but if they are part of the emit, then all of their dependency sites should be rewritten to what they are in the emit.

[weswigham]

@rbuckton we recently added the bundledPackageName flag, allowing users to specify pretty much exactly what you ask, to fix declaration emit paths in a similar way, perhaps it should be required for those module emit modes (even without declarations), too?