Use getters to define live export bindings refresh #35967
Conversation
| for (var p in m) b(p); | ||
| function b(p) { | ||
| if (!exports.hasOwnProperty(p)) | ||
| Object.create |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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.
There was a problem hiding this comment.
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;
};
})();There was a problem hiding this comment.
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;
});There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@weswigham I don't mind at all. Thanks for moving this forward 👍 |
| @@ -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; | |||
There was a problem hiding this comment.
This needs to use Object.defineProperty (with value instead of get), because assignments will fail if mod has a default property
|
@ajafff @DanielRosenwasser @rbuckton I've updated our emit to use an unscoped In addition, our es3 emit for I think that implements all the suggestions from the previous thread and this one? Once we're OK with it, I can open an accompanying tslib PR. |
|
Just to be clear, this needs to wait until 3.9 unless we're okay with a break in the RC and not the beta (which I'd prefer we didn't) |
|
I figured as much (but that just means we shan't merge it for about 2 weeks); but we do need to look at getting this in soon - it's one of the blockers to migrating us to modules. |
|
@weswigham @rbuckton there's another thing that needs to be fixed in the module transform: uninitialized bindings are not assigned to the // @filename: foo.ts
export let foo: number; // this doesn't write 'exports.foo' until the first call to 'setFoo'
export function setFoo(v: number) {
foo = v;
}
// @filename: bar.ts
export * from './foo'; // this only re-exports 'setFoo' because 'foo' is not present yet
// @filename: baz.ts
import {foo, setFoo} from './bar';
setFoo(1);
console.log(foo); // should print '1', currently prints 'undefined'To achieve this, exported variables without initializer need to be initialized with |
|
@weswigham now that master branch is headed toward 3.9, is this ready to go in? @DanielRosenwasser does your feedback still apply? It looks like it was addressed but I only skimmed the comments here. |
|
I hope so; though @ajaff's remark on needing to create getters for uninitialized variables is valid (that we didn't initialize them before was odd, too, so it's not a regression, but it should probably be fixed). |
|
I think we should try to use |
|
ping @rbuckton again again~ |
Exports now have more boilerplate because of the changes in Typescript 1.9.2; * https://github.com/microsoft/TypeScript/wiki/Breaking-Changes#exports-now-use-getters-for-live-bindings * microsoft/TypeScript#35967
This is an update/resync of #33587 with the feedback from the recent reviews applied and a merge with
master. Hopefully you don't mind @mlrawlings.Fixes #12522
Fixes #6366
Fixes #33587
cc @rbuckton and @ajafff for another review pass so we can get this in.