Use getters to define live export bindings #33587

mlrawlings · 2019-09-25T05:13:30Z

When re-exporting a binding, it should remain live instead of being a snapshot. This PR uses the same approach as Babel: use getters instead of property assignments .

Summary of Changes

Add option for getters in createExportExpression

The createExportExpression has a new liveBinding argument that, if true, causes a getter to be defined rather than using a property assignment. In the cases where liveBinding is needed, this argument is passed as true. Specifically, 2 cases are handled:

export from

export { x } from “foo”;

liveBinding is passed from visitExportDeclaration

re-exporting an import

import { x } from “foo”;
export { x };

liveBinding is passed from appendExportsOfImportDeclaration → appendExportsOfDeclaration → appendExportStatement → createExportStatement

Use getters in exportStarHelper

The exportStarHelper has also been updated to define getters rather than use a property assignment to handle the following case:

export * from “foo”;

Tests

No new tests have been added as the existing suite adequately covers these cases, but the baseline for 50 of these tests have been updated to reflect the desired output.

msftclas · 2019-09-25T05:13:45Z

All CLA requirements met.

src/compiler/transformers/module/module.ts

Jessidhia · 2019-09-26T05:26:44Z

tests/baselines/reference/ambientShorthand_reExport.js

@@ -20,11 +20,19 @@ x($);
 "use strict";
 exports.__esModule = true;
 var jquery_1 = require("jquery");
-exports.x = jquery_1.x;
+Object.defineProperty(exports, "x", { enumerable: true, get: () => jquery_1.x });


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.

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.

Jessidhia · 2019-09-26T05:27:35Z

tests/baselines/reference/ambientShorthand_reExport.js

@@ -20,11 +20,19 @@ x($);
 "use strict";
 exports.__esModule = true;


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.

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;

}

tests/baselines/reference/ambientShorthand_reExport.js

Jessidhia · 2019-09-26T05:32:30Z

tests/baselines/reference/es6ExportClauseWithoutModuleSpecifierInEs5.js

 var server_4 = require("./server");
-exports.x = server_4.x;
+Object.defineProperty(exports, "x", { enumerable: true, get: () => server_4.x });


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

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.

mlrawlings · 2019-10-01T20:26:13Z

@Jessidhia @ajafff What's the next step here?

squidfunk · 2019-10-23T08:34:09Z

This needs to be fixed ASAP - the issue where this bug was first reported is already 3 (!) years old, but the TypeScript team doesn't seem to care that much. Predictable re-exports are essential, yet TypeScript doesn't support them.

weswigham · 2019-12-16T18:59:36Z

@rbuckton can we actively consider this PR? Taking it would reduce the work we'd need to do elsewhere for our own translation to modules.

rbuckton · 2019-12-16T19:29:55Z

src/compiler/transformers/module/module.ts

@@ -998,8 +998,8 @@ namespace ts {
                        setOriginalNode(
                            setTextRange(
                                createExpressionStatement(
-                                    createExportExpression(getExportName(specifier), exportedValue)
-                                ),
+                                    createExportExpression(getExportName(specifier), exportedValue, /* location */ undefined, /* liveBinding */ true)


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

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

rbuckton · 2019-12-16T19:31:55Z

src/compiler/transformers/module/module.ts

-                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, {


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).

rbuckton · 2019-12-16T19:35:14Z

tests/baselines/reference/ambientShorthand_reExport.js

@@ -20,11 +20,19 @@ x($);
 "use strict";
 exports.__esModule = true;
 var jquery_1 = require("jquery");
-exports.x = jquery_1.x;
+Object.defineProperty(exports, "x", { enumerable: true, get: () => jquery_1.x });


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).

rbuckton · 2019-12-16T19:36:31Z

src/compiler/transformers/module/module.ts

@@ -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);


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

mlrawlings · 2019-12-16T21:37:53Z

@rbuckton I've made the changes to not break the ES3 target with this PR:

createExportExpression now checks language version and only respects liveBinding if the target is not ES3.
exportStarHelper checks for Object.create as suggested and uses plain property assignment if the property is falsy.

What would be your thoughts on moving the declaration of exportStarHelper into the transformModule function? It would then have access to languageVersion and would remove the need for a runtime check.

rbuckton · 2019-12-16T21:35:26Z

tests/baselines/reference/declarationEmitReexportedSymlinkReference.js

-    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, {


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

rbuckton · 2019-12-16T21:35:45Z

tests/baselines/reference/declarationEmitReexportedSymlinkReference3.js

-    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, {


This test result is also out of date

rbuckton · 2019-12-16T21:38:14Z

src/compiler/transformers/module/module.ts

+                        createLiteral(name),
+                        createObjectLiteral([
+                            createPropertyAssignment("enumerable", createLiteral(/*value*/ true)),
+                            createPropertyAssignment("get", createArrowFunction(


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.

rbuckton · 2019-12-16T21:39:33Z

tests/baselines/reference/transformApi/transformsCorrectly.transformAddCommentToImport.js

-    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, {


This test result is also out of date

rbuckton · 2019-12-16T21:56:29Z

The runtime check is still somewhat useful if you are targeting ES3 but end up running in an ES5+ environment. We also need to make this change to the __exportStar helper in tslib for use with the --importHelpers flag, and we don't want to have separate ES3 and ES5+ versions of that external helper.

rbuckton · 2019-12-16T22:00:52Z

Perhaps we should rename the __export helper to __exportStar and implement a new __export helper for single exports that has the same feature-test for down leveling:

// NOTE: below does feature-test once for whole file on the first call to `__export`
function __export(m, p) {
  __export = Object.create
    ? function (m, p) { Object.defineProperty(exports, p, { enumerable: false, get: function() { return m[p]; } }); } 
    : function (m, p) { exports[p] = m[p]; };
  __export(m, p);
}

Then we could cut down on the repetition of the Object.defineProperty calls a bit, and reimplement __exportStar in terms of __export:

function __exportStar(m) {
  for (var p in m) if (!exports.hasOwnProperty(p)) __export(m, p);
}

@weswigham, @DanielRosenwasser any thoughts on this? We'd also have to do the same for tslib.

weswigham · 2019-12-16T22:38:15Z

any thoughts on this?

Right now __export isn't a real helper, right? Since it closes over exports and always has to be redefined in scope. At least for now, I think not breaking that is probably correct? Minimally, I'd do that break in a separate PR.

rbuckton · 2019-12-16T22:54:33Z

__export is a "scoped" helper, meaning that it closes over something in the lexical scope, although when we use --importHelpers we use an "unscoped" helper named __exportStar that doesn't close over anything in the lexical scope.

weswigham · 2019-12-17T00:45:46Z

@mlrawlings you probably also need to merge in master to get CI - I'm pretty sure CI refuses to run while there's a merge conflict.

weswigham · 2019-12-17T00:47:49Z

@rbuckton eh. It might be OK to do that. I'd still rather do that refactoring separately from the emit feature of proper reexported live bindings, just to reduce the upfront review/design work on @mlrawlings here.

DanielRosenwasser · 2019-12-23T19:45:16Z

We spoke about this at our last design meeting and said we wanted it for 3.8. @mlrawlings can you update this branch to fix up the merge conflicts?

ajafff

This PR also needs to update __importStar as it currently copies the values of each property of the imported namespace onto another object. It should use getters as well.

DanielRosenwasser · 2020-01-02T22:48:47Z

src/compiler/transformers/module/module.ts

+                function b(p) {
+                    if (!exports.hasOwnProperty(p))
+                        Object.create
+                            ? Object.defineProperty(exports, p, {


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.

sandersn

Looks like there are a few outstanding changes requested.

To help me track our PR backlog, I'm going to request changes officially.

weswigham · 2020-02-03T18:53:10Z

@sandersn this is superseded by #35967, which is an update of this with feedback taken into account.

use getters to define live export bindings

a8d6c2b

ajafff suggested changes Sep 25, 2019

View reviewed changes

src/compiler/transformers/module/module.ts Outdated Show resolved Hide resolved

fix scoping in export* helper

27941d0

Jessidhia reviewed Sep 26, 2019

View reviewed changes

mlrawlings mentioned this pull request Sep 26, 2019

use getters for exportStar helper microsoft/tslib#78

Closed

rbuckton requested changes Dec 16, 2019

View reviewed changes

Object.defineProperty cannot be used in ES3 target

0b2cf9b

rbuckton requested changes Dec 16, 2019

View reviewed changes

DanielRosenwasser added Breaking Change Would introduce errors in existing code Update Docs on Next Release Indicates that this PR affects docs labels Dec 20, 2019

DanielRosenwasser mentioned this pull request Dec 20, 2019

Design Meeting Notes, 12/20/2019 #35807

Closed

ajafff suggested changes Dec 30, 2019

View reviewed changes

weswigham mentioned this pull request Jan 2, 2020

Use getters to define live export bindings refresh #35967

Merged

DanielRosenwasser reviewed Jan 2, 2020

View reviewed changes

sandersn requested changes Feb 3, 2020

View reviewed changes

weswigham closed this Feb 3, 2020

mwpenny mentioned this pull request Dec 4, 2020

New Proposed API checking causes extensions to fail to load when using "import * as vscode" with TypeScript < 3.9 microsoft/vscode#111913

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use getters to define live export bindings #33587

Use getters to define live export bindings #33587

mlrawlings commented Sep 25, 2019 •

edited by weswigham

Loading

msftclas commented Sep 25, 2019 •

edited

Loading

Jessidhia Sep 26, 2019

mlrawlings Sep 26, 2019

Jessidhia Sep 26, 2019

mlrawlings Sep 26, 2019

Jessidhia Sep 26, 2019

mlrawlings Sep 26, 2019

mlrawlings commented Oct 1, 2019

squidfunk commented Oct 23, 2019

weswigham commented Dec 16, 2019

rbuckton Dec 16, 2019

ExE-Boss Dec 21, 2019

rbuckton Dec 16, 2019

rbuckton Dec 16, 2019

rbuckton Dec 16, 2019

mlrawlings commented Dec 16, 2019 •

edited

Loading

rbuckton Dec 16, 2019

rbuckton Dec 16, 2019

rbuckton Dec 16, 2019

rbuckton Dec 16, 2019

rbuckton commented Dec 16, 2019

rbuckton commented Dec 16, 2019 •

edited

Loading

weswigham commented Dec 16, 2019

rbuckton commented Dec 16, 2019

weswigham commented Dec 17, 2019

weswigham commented Dec 17, 2019

DanielRosenwasser commented Dec 23, 2019 •

edited

Loading

ajafff left a comment

DanielRosenwasser Jan 2, 2020

sandersn left a comment

weswigham commented Feb 3, 2020

		@@ -20,11 +20,19 @@ x($);
		"use strict";
		exports.__esModule = true;

	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;
	}

Use getters to define live export bindings #33587

Use getters to define live export bindings #33587

Conversation

mlrawlings commented Sep 25, 2019 • edited by weswigham Loading

Summary of Changes

Add option for getters in createExportExpression

export from

re-exporting an import

Use getters in exportStarHelper

Tests

msftclas commented Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlrawlings commented Oct 1, 2019

squidfunk commented Oct 23, 2019

weswigham commented Dec 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlrawlings commented Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton commented Dec 16, 2019

rbuckton commented Dec 16, 2019 • edited Loading

weswigham commented Dec 16, 2019

rbuckton commented Dec 16, 2019

weswigham commented Dec 17, 2019

weswigham commented Dec 17, 2019

DanielRosenwasser commented Dec 23, 2019 • edited Loading

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

weswigham commented Feb 3, 2020

mlrawlings commented Sep 25, 2019 •

edited by weswigham

Loading

msftclas commented Sep 25, 2019 •

edited

Loading

mlrawlings commented Dec 16, 2019 •

edited

Loading

rbuckton commented Dec 16, 2019 •

edited

Loading

DanielRosenwasser commented Dec 23, 2019 •

edited

Loading