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

Use getters to define live export bindings refresh #35967

Merged
merged 18 commits into from
Feb 24, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 56 additions & 12 deletions src/compiler/transformers/module/module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1009,7 +1009,7 @@ namespace ts {
setOriginalNode(
setTextRange(
createExpressionStatement(
createExportExpression(getExportName(specifier), exportedValue)
createExportExpression(getExportName(specifier), exportedValue, /* location */ undefined, /* liveBinding */ true)
),
specifier),
specifier
Expand Down Expand Up @@ -1343,7 +1343,7 @@ namespace ts {

case SyntaxKind.NamedImports:
for (const importBinding of namedBindings.elements) {
statements = appendExportsOfDeclaration(statements, importBinding);
statements = appendExportsOfDeclaration(statements, importBinding, /* liveBinding */ true);
}

break;
Expand Down Expand Up @@ -1453,12 +1453,12 @@ namespace ts {
* appended.
* @param decl The declaration to export.
*/
function appendExportsOfDeclaration(statements: Statement[] | undefined, decl: Declaration): Statement[] | undefined {
function appendExportsOfDeclaration(statements: Statement[] | undefined, decl: Declaration, liveBinding?: boolean): Statement[] | undefined {
const name = getDeclarationName(decl);
const exportSpecifiers = currentModuleInfo.exportSpecifiers.get(idText(name));
if (exportSpecifiers) {
for (const exportSpecifier of exportSpecifiers) {
statements = appendExportStatement(statements, exportSpecifier.name, name, /*location*/ exportSpecifier.name);
statements = appendExportStatement(statements, exportSpecifier.name, name, /*location*/ exportSpecifier.name, /* allowComments */ undefined, liveBinding);
}
}
return statements;
Expand All @@ -1476,8 +1476,8 @@ namespace ts {
* @param location The location to use for source maps and comments for the export.
* @param allowComments Whether to allow comments on the export.
*/
function appendExportStatement(statements: Statement[] | undefined, exportName: Identifier, expression: Expression, location?: TextRange, allowComments?: boolean): Statement[] | undefined {
statements = append(statements, createExportStatement(exportName, expression, location, allowComments));
function appendExportStatement(statements: Statement[] | undefined, exportName: Identifier, expression: Expression, location?: TextRange, allowComments?: boolean, liveBinding?: boolean): Statement[] | undefined {
statements = append(statements, createExportStatement(exportName, expression, location, allowComments, liveBinding));
return statements;
}

Expand Down Expand Up @@ -1518,8 +1518,8 @@ namespace ts {
* @param location The location to use for source maps and comments for the export.
* @param allowComments An optional value indicating whether to emit comments for the statement.
*/
function createExportStatement(name: Identifier, value: Expression, location?: TextRange, allowComments?: boolean) {
const statement = setTextRange(createExpressionStatement(createExportExpression(name, value)), location);
function createExportStatement(name: Identifier, value: Expression, location?: TextRange, allowComments?: boolean, liveBinding?: boolean) {
const statement = setTextRange(createExpressionStatement(createExportExpression(name, value, /* location */ undefined, liveBinding)), location);
startOnNewLine(statement);
if (!allowComments) {
setEmitFlags(statement, EmitFlags.NoComments);
Expand All @@ -1535,9 +1535,31 @@ namespace ts {
* @param value The exported value.
* @param location The location to use for source maps and comments for the export.
*/
function createExportExpression(name: Identifier, value: Expression, location?: TextRange) {
function createExportExpression(name: Identifier, value: Expression, location?: TextRange, liveBinding?: boolean) {
return setTextRange(
createAssignment(
liveBinding && languageVersion !== ScriptTarget.ES3 ? createCall(
createPropertyAccess(
createIdentifier("Object"),
"defineProperty"
),
/*typeArguments*/ undefined,
[
createIdentifier("exports"),
createLiteral(name),
createObjectLiteral([
createPropertyAssignment("enumerable", createLiteral(/*value*/ true)),
createPropertyAssignment("get", createFunctionExpression(
/*modifiers*/ undefined,
/*asteriskToken*/ undefined,
/*name*/ undefined,
/*typeParameters*/ undefined,
/*parameters*/ [],
/*type*/ undefined,
createBlock([createReturn(value)])
))
])
]
) : createAssignment(
createPropertyAccess(
createIdentifier("exports"),
getSynthesizedClone(name)
Expand Down Expand Up @@ -1819,7 +1841,18 @@ namespace ts {
scoped: true,
text: `
function __export(m) {
for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
for (var p in m) b(p);
function b(p) {
if (!exports.hasOwnProperty(p))
Object.create
? Object.defineProperty(exports, p, {
enumerable: true,
get: function () {
return m[p];
}
})
: (exports[p] = m[p]);
}
}`
};

Expand Down Expand Up @@ -1847,9 +1880,20 @@ namespace ts {
var __importStar = (this && this.__importStar) || function (mod) {
if (mod && mod.__esModule) return mod;
var result = {};
if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) result[k] = mod[k];
if (mod != null) for (var k in mod) b(k);
result["default"] = mod;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to use Object.defineProperty (with value instead of get), because assignments will fail if mod has a default property

return result;
function b(p) {
if (Object.hasOwnProperty.call(mod, p))
Object.create
? Object.defineProperty(result, p, {
enumerable: true,
get: function () {
return mod[p];
}
})
: (result[p] = mod[p]);
}
};`
};

Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/ambientShorthand_reExport.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,18 @@ exports.x = jquery_1.x;
//// [reExportAll.js]
"use strict";
function __export(m) {
for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
for (var p in m) b(p);
function b(p) {
if (!exports.hasOwnProperty(p))
Object.create
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance you can update this for this feedback? #33587 (comment)

Not a fan of how this gets checked on every iteration of the loop. In other places, we just decide up front what the helper is going to be and use that.

Copy link
Member Author

@weswigham weswigham Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but it'd double the helper size. In the case of __importStar, we'd go from

var __importStar = (this && this.__importStar) || function (mod) {
    if (mod && mod.__esModule) return mod;
    var result = {};
    if (mod != null) for (var k in mod) b(k);
    result["default"] = mod;
    return result;
    function b(p) {
        if (Object.hasOwnProperty.call(mod, p))
            Object.create
                ? Object.defineProperty(result, p, {
                      enumerable: true,
                      get: function() {
                          return mod[p];
                      }
                  })
                : (result[p] = mod[p]);
    }
};

to

var __importStar = (this && this.__importStar) || (Object.create ? (function (mod) {
    if (mod && mod.__esModule) return mod;
    var result = {};
    if (mod != null) for (var k in mod) b(k);
    result["default"] = mod;
    return result;
    function b(p) {
        if (Object.hasOwnProperty.call(mod, p))
            Object.defineProperty(result, p, {
                enumerable: true,
                get: function() {
                    return mod[p];
                }
            });
    }
}) : (function (mod) {
    if (mod && mod.__esModule) return mod;
    var result = {};
    if (mod != null) for (var k in mod) b(k);
    result["default"] = mod;
    return result;
    function b(p) {
        if (Object.hasOwnProperty.call(mod, p))
             result[p] = mod[p];
    }
}));

(you can compress it a bit by moving the property definition into another helper (eg __setBinding) but that's way more complexity, and if we wanted to do that, I'd rather do it in a followup)

I think doing the (hopefully fast) check on the (executed only a few times at the top level) import/export helpers is a fair enough tradeoff to make in our default helper implementations. Really: If you have strong execution speed or size concerns, you'll be providing custom helpers that target exactly the runtime you're building for, without any runtime feature testing at all.

For a point of comparison: Babel's helper checks for Object.defineProperty on every call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(you can compress it a bit by moving the property definition into another helper (eg __setBinding) but that's way more complexity, and if we wanted to do that, I'd rather do it in a followup)

Yeah, that's what I had in mind. But I guess we can improve in another PR if users ask.

Copy link
Member

@rbuckton rbuckton Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also could have done this:

var __importStar = (this && this.__importStar) || (function () {
    var setModuleDefault = (Object.create ? (function(o, v) {
        Object.defineProperty(o, "default", { enumerable: true, value: v });
    }) : function(o, v) {
        o["default"] = v;
    });
    return function (mod) {
        if (mod && mod.__esModule) return mod;
        var result = {};
        if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) __createBinding(result, mod, k);
        setModuleDefault(result, mod);
        return result;
    };
})();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or this:

var __importStar = (this && this.__importStar) || (function (setModuleDefault) {
    return function (mod) {
        if (mod && mod.__esModule) return mod;
        var result = {};
        if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) __createBinding(result, mod, k);
        setModuleDefault(result, mod);
        return result;
    };
})(Object.create ? (function(o, v) {
    Object.defineProperty(o, "default", { enumerable: true, value: v });
}) : function(o, v) {
    o["default"] = v;
});

Copy link
Member Author

@weswigham weswigham Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For __createBinding at least, it's used by both __importStar and __exportStar, though, so that one actually saves space by being a separate helper.

For setModuleDefault... eh, I guess? Still seems nicer to be separate helpers - definitely looks nicer in tslib.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already do that for __extends in tslib. In src/compiler/transformers/es2015.ts we have extendsStatics inline inside of __extends, but in tslib they are two distinct functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also defer its value until later in case a polyfill adds Object.setPrototypeOf, but you can't properly polyfill Object.defineProperty due to IE8.

? Object.defineProperty(exports, p, {
enumerable: true,
get: function () {
return m[p];
}
})
: (exports[p] = m[p]);
}
}
exports.__esModule = true;
__export(require("jquery"));
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/commentsOnRequireStatement.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ Object.defineProperty(exports, "__esModule", { value: true });
// blah
// blah
var _0_1 = require("./0");
exports.subject = _0_1.subject;
Object.defineProperty(exports, "subject", { enumerable: true, get: function () { return _0_1.subject; } });
rbuckton marked this conversation as resolved.
Show resolved Hide resolved
/* blah1 */
var _1_1 = require("./1");
exports.subject1 = _1_1.subject1;
Object.defineProperty(exports, "subject1", { enumerable: true, get: function () { return _1_1.subject1; } });
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ exports.bar = bar;
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
var utils_1 = require("./utils");
exports.bar = utils_1.bar;
Object.defineProperty(exports, "bar", { enumerable: true, get: function () { return utils_1.bar; } });
utils_1.foo();
var obj;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,18 @@ exports.ADMIN = pkg2_1.MetadataAccessor.create('1');
//// [index.js]
"use strict";
function __export(m) {
for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
for (var p in m) b(p);
function b(p) {
if (!exports.hasOwnProperty(p))
Object.create
? Object.defineProperty(exports, p, {
enumerable: true,
get: function () {
return m[p];
}
})
: (exports[p] = m[p]);
}
}
Object.defineProperty(exports, "__esModule", { value: true });
__export(require("./keys"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,18 @@ exports.ADMIN = pkg2_1.MetadataAccessor.create('1');
//// [index.js]
"use strict";
function __export(m) {
for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
for (var p in m) b(p);
function b(p) {
if (!exports.hasOwnProperty(p))
Object.create
? Object.defineProperty(exports, p, {
enumerable: true,
get: function () {
return m[p];
}
})
: (exports[p] = m[p]);
}
}
Object.defineProperty(exports, "__esModule", { value: true });
__export(require("./keys"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,18 @@ exports.ADMIN = pkg2_1.MetadataAccessor.create('1');
//// [index.js]
"use strict";
function __export(m) {
for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
for (var p in m) b(p);
function b(p) {
if (!exports.hasOwnProperty(p))
Object.create
? Object.defineProperty(exports, p, {
enumerable: true,
get: function () {
return m[p];
}
})
: (exports[p] = m[p]);
}
}
Object.defineProperty(exports, "__esModule", { value: true });
__export(require("./keys"));
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/doubleUnderscoreExportStarConflict.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,18 @@ exports.__foo = __foo;
//// [index.js]
"use strict";
function __export(m) {
for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
for (var p in m) b(p);
function b(p) {
if (!exports.hasOwnProperty(p))
Object.create
? Object.defineProperty(exports, p, {
enumerable: true,
get: function () {
return m[p];
}
})
: (exports[p] = m[p]);
}
}
exports.__esModule = true;
__export(require("./b"));
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/es6ExportAllInEs5.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,18 @@ exports.x = 10;
//// [client.js]
"use strict";
function __export(m) {
for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
for (var p in m) b(p);
function b(p) {
if (!exports.hasOwnProperty(p))
Object.create
? Object.defineProperty(exports, p, {
enumerable: true,
get: function () {
return m[p];
}
})
: (exports[p] = m[p]);
}
}
Object.defineProperty(exports, "__esModule", { value: true });
__export(require("./server"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ exports.x = 10;
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
var server_1 = require("./server");
exports.c = server_1.c;
Object.defineProperty(exports, "c", { enumerable: true, get: function () { return server_1.c; } });
var server_2 = require("./server");
exports.c2 = server_2.c;
Object.defineProperty(exports, "c2", { enumerable: true, get: function () { return server_2.c; } });
var server_3 = require("./server");
exports.instantiatedModule = server_3.m;
Object.defineProperty(exports, "instantiatedModule", { enumerable: true, get: function () { return server_3.m; } });
var server_4 = require("./server");
exports.x = server_4.x;
Object.defineProperty(exports, "x", { enumerable: true, get: function () { return server_4.x; } });


//// [server.d.ts]
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/es6ExportEqualsInterop.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,18 @@ export * from "class-module";
"use strict";
/// <reference path="modules.d.ts"/>
function __export(m) {
for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
for (var p in m) b(p);
function b(p) {
if (!exports.hasOwnProperty(p))
Object.create
? Object.defineProperty(exports, p, {
enumerable: true,
get: function () {
return m[p];
}
})
: (exports[p] = m[p]);
}
}
exports.__esModule = true;
var z2 = require("variable");
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/esModuleInterop.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,20 @@ var __importDefault = (this && this.__importDefault) || function (mod) {
var __importStar = (this && this.__importStar) || function (mod) {
if (mod && mod.__esModule) return mod;
var result = {};
if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) result[k] = mod[k];
if (mod != null) for (var k in mod) b(k);
result["default"] = mod;
return result;
function b(p) {
if (Object.hasOwnProperty.call(mod, p))
Object.create
? Object.defineProperty(result, p, {
enumerable: true,
get: function () {
return mod[p];
}
})
: (result[p] = mod[p]);
}
};
exports.__esModule = true;
var hybrid_1 = require("./hybrid");
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/esModuleInteropImportCall.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,20 @@ import("./foo").then(f => {
var __importStar = (this && this.__importStar) || function (mod) {
if (mod && mod.__esModule) return mod;
var result = {};
if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) result[k] = mod[k];
if (mod != null) for (var k in mod) b(k);
result["default"] = mod;
return result;
function b(p) {
if (Object.hasOwnProperty.call(mod, p))
Object.create
? Object.defineProperty(result, p, {
enumerable: true,
get: function () {
return mod[p];
}
})
: (result[p] = mod[p]);
}
};
Promise.resolve().then(function () { return __importStar(require("./foo")); }).then(function (f) {
f["default"];
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/esModuleInteropImportNamespace.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,20 @@ foo.default;
var __importStar = (this && this.__importStar) || function (mod) {
if (mod && mod.__esModule) return mod;
var result = {};
if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) result[k] = mod[k];
if (mod != null) for (var k in mod) b(k);
result["default"] = mod;
return result;
function b(p) {
if (Object.hasOwnProperty.call(mod, p))
Object.create
? Object.defineProperty(result, p, {
enumerable: true,
get: function () {
return mod[p];
}
})
: (result[p] = mod[p]);
}
};
exports.__esModule = true;
var foo = __importStar(require("./foo"));
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/esModuleInteropNamedDefaultImports.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,20 @@ var __importDefault = (this && this.__importDefault) || function (mod) {
var __importStar = (this && this.__importStar) || function (mod) {
if (mod && mod.__esModule) return mod;
var result = {};
if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) result[k] = mod[k];
if (mod != null) for (var k in mod) b(k);
result["default"] = mod;
return result;
function b(p) {
if (Object.hasOwnProperty.call(mod, p))
Object.create
? Object.defineProperty(result, p, {
enumerable: true,
get: function () {
return mod[p];
}
})
: (result[p] = mod[p]);
}
};
exports.__esModule = true;
var mod_1 = __importDefault(require("./mod"));
Expand Down
Loading