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 2 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
53 changes: 41 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 ? 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,15 @@ 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.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 is not down-level compatible with ES3. You should instead feature-test for Object.create (since Object.defineProperty exists in some versions of IE that only supported it for DOM elements, but Object.create did not exist until full ES5 support landed).

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

Expand Down
12 changes: 10 additions & 2 deletions tests/baselines/reference/ambientShorthand_reExport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,19 @@ x($);
"use strict";
exports.__esModule = true;
Copy link

Choose a reason for hiding this comment

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

If you're going with defineProperty then the __esModule property also probably should be defined (non-enumerable, non-configurable, non-writable); as it is it's all three.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there's some code that already handles using Object.defineProperty when the script target is not ES3:

TypeScript/src/compiler/transformers/module/module.ts

Lines 1451 to 1478 in 27941d0

function createUnderscoreUnderscoreESModule() {
let statement: Statement;
if (languageVersion === ScriptTarget.ES3) {
statement = createExpressionStatement(
createExportExpression(
createIdentifier("__esModule"),
createLiteral(/*value*/ true)
)
);
}
else {
statement = createExpressionStatement(
createCall(
createPropertyAccess(createIdentifier("Object"), "defineProperty"),
/*typeArguments*/ undefined,
[
createIdentifier("exports"),
createLiteral("__esModule"),
createObjectLiteral([
createPropertyAssignment("value", createLiteral(/*value*/ true))
])
]
)
);
}
setEmitFlags(statement, EmitFlags.CustomPrologue);
return statement;
}

var jquery_1 = require("jquery");
exports.x = jquery_1.x;
Object.defineProperty(exports, "x", { enumerable: true, get: () => jquery_1.x });
Copy link

Choose a reason for hiding this comment

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

In cases like this it actually should be possible to hoist the Object.defineProperty call itself to happen before any of the other require calls.

This might make circular imports more compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Digging into this, it looks like you are correct - this should be hoisted. But so should every exported value, whether it's defined as a getter or not. I'm thinking all these cases should be handled as a separate PR.

As it stands, if the property is accessed but the module from which it was re-exporting the value has not been loaded, we won't get the value in either case. The only thing that's affected is enumerability, which is already an issue without this PR. This PR is a step in the right direction and solves #12522. The hoisting is a separate issue IMO.

Copy link
Member

Choose a reason for hiding this comment

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

I believe this test and every other test that doesn't have a // @target defaults to ES3, so many of these test results are actually incorrect as they would not work in the target environment (where Object.defineProperty is not defined).

//// [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.defineProperty(exports, p, {
enumerable: true,
get: function () {
return 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/constEnumPreserveEmitReexport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,4 @@ exports["default"] = ConstEnum_1.MyConstEnum;
"use strict";
exports.__esModule = true;
var ConstEnum_1 = require("./ConstEnum");
exports["default"] = ConstEnum_1.MyConstEnum;
Object.defineProperty(exports, "default", { enumerable: true, get: () => ConstEnum_1.MyConstEnum });
2 changes: 1 addition & 1 deletion tests/baselines/reference/declarationEmitDefaultExportWithStaticAssignment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ Example.Foo = foo_1.Foo;
"use strict";
exports.__esModule = true;
var foo_1 = require("./foo");
exports.Foo = foo_1.Foo;
Object.defineProperty(exports, "Foo", { enumerable: true, get: () => foo_1.Foo });
function Example() { }
exports["default"] = Example;
Example.Foo = foo_1.Foo;
Expand Down
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
10 changes: 9 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,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, {
enumerable: true,
get: function () {
return 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
2 changes: 1 addition & 1 deletion tests/baselines/reference/declarationMapsMultifile.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion tests/baselines/reference/declarationMapsOutFile.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions tests/baselines/reference/destructuredDeclarationEmit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ exports.arr = arr;
"use strict";
exports.__esModule = true;
var foo_1 = require("./foo");
exports.foo = foo_1.foo;
exports.arr = foo_1.arr;
Object.defineProperty(exports, "foo", { enumerable: true, get: () => foo_1.foo });
Object.defineProperty(exports, "arr", { enumerable: true, get: () => foo_1.arr });
var baz = foo_1.foo.bar, bat = foo_1.foo.bat, _a = foo_1.foo.bam.bork, ibar = _a.bar, ibaz = _a.baz;
exports.baz = baz;
exports.ibaz = ibaz;
Expand Down
10 changes: 9 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,15 @@ 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.defineProperty(exports, p, {
enumerable: true,
get: function () {
return m[p];
}
});
}
}
exports.__esModule = true;
__export(require("./b"));
Expand Down
10 changes: 9 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,15 @@ 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.defineProperty(exports, p, {
enumerable: true,
get: function () {
return 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
30 changes: 19 additions & 11 deletions 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,15 @@ 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.defineProperty(exports, p, {
enumerable: true,
get: function () {
return m[p];
}
});
}
}
exports.__esModule = true;
var z2 = require("variable");
Expand Down Expand Up @@ -274,25 +282,25 @@ class_1.a;
class_module_1.a;
// named export
var interface_2 = require("interface");
exports.a1 = interface_2.a;
Object.defineProperty(exports, "a1", { enumerable: true, get: () => interface_2.a });
var variable_2 = require("variable");
exports.a2 = variable_2.a;
Object.defineProperty(exports, "a2", { enumerable: true, get: () => variable_2.a });
var interface_variable_2 = require("interface-variable");
exports.a3 = interface_variable_2.a;
Object.defineProperty(exports, "a3", { enumerable: true, get: () => interface_variable_2.a });
var module_2 = require("module");
exports.a4 = module_2.a;
Object.defineProperty(exports, "a4", { enumerable: true, get: () => module_2.a });
var interface_module_2 = require("interface-module");
exports.a5 = interface_module_2.a;
Object.defineProperty(exports, "a5", { enumerable: true, get: () => interface_module_2.a });
var variable_module_2 = require("variable-module");
exports.a6 = variable_module_2.a;
Object.defineProperty(exports, "a6", { enumerable: true, get: () => variable_module_2.a });
var function_2 = require("function");
exports.a7 = function_2.a;
Object.defineProperty(exports, "a7", { enumerable: true, get: () => function_2.a });
var function_module_2 = require("function-module");
exports.a8 = function_module_2.a;
Object.defineProperty(exports, "a8", { enumerable: true, get: () => function_module_2.a });
var class_2 = require("class");
exports.a9 = class_2.a;
Object.defineProperty(exports, "a9", { enumerable: true, get: () => class_2.a });
var class_module_2 = require("class-module");
exports.a0 = class_module_2.a;
Object.defineProperty(exports, "a0", { enumerable: true, get: () => class_module_2.a });
__export(require("variable"));
__export(require("interface-variable"));
__export(require("module"));
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/esModuleInteropTslibHelpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,5 @@ exports.Foo3 = Foo3;
exports.__esModule = true;
var tslib_1 = require("tslib");
var path_1 = tslib_1.__importStar(require("path"));
exports.Bar = path_1.Bar;
Object.defineProperty(exports, "Bar", { enumerable: true, get: () => path_1.Bar });
path_1["default"]("", "../");
6 changes: 3 additions & 3 deletions tests/baselines/reference/exportDeclarationWithModuleSpecifierNameOnNextLine1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,17 @@ exports.x = "x";
"use strict";
exports.__esModule = true;
var t1_1 = require("./t1");
exports.x = t1_1.x;
Object.defineProperty(exports, "x", { enumerable: true, get: () => t1_1.x });
//// [t3.js]
"use strict";
exports.__esModule = true;
//// [t4.js]
"use strict";
exports.__esModule = true;
var t1_1 = require("./t1");
exports.a = t1_1.x;
Object.defineProperty(exports, "a", { enumerable: true, get: () => t1_1.x });
//// [t5.js]
"use strict";
exports.__esModule = true;
var t1_1 = require("./t1");
exports.a = t1_1.x;
Object.defineProperty(exports, "a", { enumerable: true, get: () => t1_1.x });
10 changes: 9 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,15 @@ 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.defineProperty(exports, p, {
enumerable: true,
get: function () {
return m[p];
}
});
}
}
Object.defineProperty(exports, "__esModule", { value: true });
__export(t1_1);
Expand Down
Loading