microsoft / tslib

Runtime library for TypeScript helpers.
BSD Zero Clause License
1.26k stars 128 forks source link

set the __esModule marker in createExporter #28

Closed aluanhaddad closed 7 years ago

aluanhaddad commented 7 years ago

resolves #27

xtuc commented 7 years ago

I'm just wondering where is the code to handle ESM imports? You'll need something like the Babel require interop helper.

aluanhaddad commented 7 years ago

@xtuc the idea here is for tslib to work with exactly the sort of code in babel/helpers.interopRequireDefault. It is the responsibility of the loader to check for the __esModule flag.

However, the current behavior of tslib/tslib.js when imported by an interop aware function such as interopRequireDefault is that the all of the helpers get lifted into the default binding on the module namespace object. This causes the code generated by TypeScript to fail since it looks like

import * as tslib_1 from "tslib";

export function fetchStuff() {
    return tslib_1.__awaiter(this, void 0, void 0, function () {
        var response, data;
        return tslib_1.__generator(this, function (_a) {
            switch (_a.label) {
                case 0: return [4 /*yield*/, fetch('api/stuff')];
                case 1:
                    response = _a.sent();
                    return [4 /*yield*/, response.json()];
                case 2:
                    data = _a.sent();
                    return [2 /*return*/, tslib_1.__assign({}, data)];
            }
        });
    });
}

This fails at runtime since tslib_1.__awaiter and so on are undefined.

However, I think it possible that this behavior may be more an artifact of how the code was written, by hand, as opposed to transpiled from tslib.es6.js.

If you run

> tsc tslib.es6.js --allowJs --module umd --target es3

the result will contain the __esModule flag. Now that is not a viable way to build this as the createExporter function and other aspects of this library are clearly hand-authored, but I think the intent lines up to where the flag should be present.

I am just speculating as to intent of course.

aluanhaddad commented 7 years ago

Any thoughts on this change?

DanielRosenwasser commented 7 years ago

@rbuckton when you get the chance, can you take a look?

rbuckton commented 7 years ago

I'm not certain why, but your last commit it's causing the diff to show a larger number of unrelated changes.

aluanhaddad commented 7 years ago

Ah, I see I synced my fork. Sorry about that. Let me fix it up

aluanhaddad commented 7 years ago

@rbuckton my apologies. The diff should be clean now. Thank you.

aluanhaddad commented 7 years ago

Updated to use feature detection as per review.

rbuckton commented 7 years ago

Thanks for the contribution!

aluanhaddad commented 7 years ago

@rbuckton thank you for the review! ❤️

aluanhaddad commented 7 years ago

Sorry for the messy commits I was trying to rebase those away but they had already been merged.