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.

Sign up for GitHub

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

Jump to bottom

Use getters to define live export bindings #33587

Closed
wants to merge 3 commits into from
Closed

Use getters to define live export bindings #33587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a8d6c2b
use getters to define live export bindings
mlrawlings Sep 24, 2019
27941d0
fix scoping in export* helper
mlrawlings Sep 25, 2019
0b2cf9b
Object.defineProperty cannot be used in ES3 target
mlrawlings Dec 16, 2019
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
56 changes: 44 additions & 12 deletions src/compiler/transformers/module/module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,8 +998,8 @@ namespace ts {
setOriginalNode(
setTextRange(
createExpressionStatement(
createExportExpression(getExportName(specifier), exportedValue)
),
createExportExpression(getExportName(specifier), exportedValue, /* location */ undefined, /* liveBinding */ true)
Copy link
Member

Choose a reason for hiding this comment

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

Will unconditionally making this a live binding result in a defineProperty call for --target ES3?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that we want to support using defineProperty, and only fall back to property assignment when it’s not supported.

),
specifier),
specifier
)
Expand Down Expand Up @@ -1310,7 +1310,7 @@ namespace ts {

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

Choose a reason for hiding this comment

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

As above, an unconditional live binding will result in code that is invalid in an ES3 target.

}

break;
Expand Down Expand Up @@ -1420,12 +1420,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 @@ -1443,8 +1443,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 @@ -1485,8 +1485,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 @@ -1502,9 +1502,30 @@ 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", createArrowFunction(
Copy link
Member

Choose a reason for hiding this comment

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

You need to use createFunctionExpression instead of createArrowFunction. The module transform is run after all other language-specific transforms have been run, so the arrow function won't be correctly down-leveled when targeting ES5.

/*modifiers*/ undefined,
/*typeParameters*/ undefined,
/*parameters*/ [],
/*type*/ undefined,
/*equalsGreaterThanToken*/ undefined,
value
))
])
]
) : createAssignment(
createPropertyAccess(
createIdentifier("exports"),
getSynthesizedClone(name)
Expand Down Expand Up @@ -1786,7 +1807,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, {
Copy link
Member

Choose a reason for hiding this comment

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

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.

enumerable: true,
get: function() {
return m[p];
}
})
: (exports[p] = m[p]);
}
}`
};

Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/ambientShorthand_reExport.js
View file
Open in desktop
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) {
mlrawlings marked this conversation as resolved.
Show resolved Hide resolved
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("jquery"));
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/commentsOnRequireStatement.js
View file
Open in desktop
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: () => _0_1.subject });
/* blah1 */
var _1_1 = require("./1");
exports.subject1 = _1_1.subject1;
Object.defineProperty(exports, "subject1", { enumerable: true, get: () => _1_1.subject1 });
2 changes: 1 addition & 1 deletion tests/baselines/reference/declarationEmitExportDeclaration.js
View file
Open in desktop
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: () => utils_1.bar });
utils_1.foo();
var obj;

Expand Down
10 changes: 9 additions & 1 deletion tests/baselines/reference/declarationEmitReexportedSymlinkReference.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,15 @@ 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.defineProperty(exports, p, {
Copy link
Member

Choose a reason for hiding this comment

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

This test doesn't look like it was updated after the changes to the helper.

enumerable: true,
get: function () {
return m[p];
}
});
}
}
Object.defineProperty(exports, "__esModule", { value: true });
__export(require("./keys"));
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/declarationEmitReexportedSymlinkReference2.js
View file
Open in desktop
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
10 changes: 9 additions & 1 deletion tests/baselines/reference/declarationEmitReexportedSymlinkReference3.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,15 @@ 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.defineProperty(exports, p, {
Copy link
Member

Choose a reason for hiding this comment

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

This test result is also out of date

enumerable: true,
get: function () {
return m[p];
}
});
}
}
Object.defineProperty(exports, "__esModule", { value: true });
__export(require("./keys"));
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/doubleUnderscoreExportStarConflict.js
View file
Open in desktop
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
View file
Open in desktop
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
8 changes: 4 additions & 4 deletions tests/baselines/reference/es6ExportClauseWithoutModuleSpecifierInEs5.js
View file
Open in desktop
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: () => server_1.c });
var server_2 = require("./server");
exports.c2 = server_2.c;
Object.defineProperty(exports, "c2", { enumerable: true, get: () => server_2.c });
var server_3 = require("./server");
exports.instantiatedModule = server_3.m;
Object.defineProperty(exports, "instantiatedModule", { enumerable: true, get: () => server_3.m });
var server_4 = require("./server");
exports.x = server_4.x;
Object.defineProperty(exports, "x", { enumerable: true, get: () => server_4.x });
Copy link

Choose a reason for hiding this comment

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

If you do the hoisting it might be possible to write these with a single Object.defineProperties call, but that needs some state bookkeeping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned above, I think the hoisting should be a separate PR. This is something that could be considered if the bookkeeping doesn't complicate things too much. I will note that Babel does not do this, but that doesn't mean TypeScript couldn't.



//// [server.d.ts]
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/es6ExportEqualsInterop.js
View file
Open in desktop
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/exportStar-amd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,18 @@ define(["require", "exports"], function (require, exports) {
define(["require", "exports", "./t1", "./t2", "./t3"], function (require, exports, t1_1, t2_1, t3_1) {
"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(t1_1);
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/exportStar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,18 @@ exports.z = z;
//// [t4.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("./t1"));
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/exportStarForValues7.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,18 @@ define(["require", "exports"], function (require, exports) {
define(["require", "exports", "file2"], function (require, exports, file2_1) {
"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(file2_1);
Expand Down
26 changes: 24 additions & 2 deletions tests/baselines/reference/exportStarForValues8.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,18 @@ define(["require", "exports"], function (require, exports) {
define(["require", "exports", "file2", "file3"], function (require, exports, file2_1, file3_1) {
"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(file2_1);
Expand All @@ -55,7 +66,18 @@ define(["require", "exports", "file2", "file3"], function (require, exports, fil
define(["require", "exports", "file4"], function (require, exports, file4_1) {
"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(file4_1);
Expand Down
Loading