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

Conversation

weswigham
Copy link
Member

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.

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.

@mlrawlings
Copy link
Contributor

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

@weswigham
Copy link
Member Author

@ajafff @DanielRosenwasser @rbuckton

I've updated our emit to use an unscoped __exportStar helper everywhere, and made both it and the __importStar helper rely on a new __createBinding helper. __importStar also relies on a new __setModuleDefault helper to appropriately handle the define vs assignment downlevel for the default member assignment (it could be inlined into __importStar if you're willing to feature test on every invocation, but I think it reads better extracted into its own helper).

In addition, our es3 emit for export {x} from "foo" uses the __createBinding helper, rather than emitting an assignment (so live bindings for those members are appropriately feature tested). Our es5 emit is still an inline Object.defineProperty call.

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.

@DanielRosenwasser
Copy link
Member

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)

@weswigham
Copy link
Member Author

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.

@ajafff
Copy link
Contributor

ajafff commented Jan 21, 2020

@weswigham @rbuckton there's another thing that needs to be fixed in the module transform: uninitialized bindings are not assigned to the exports object until the first assignment.
Since __importStar and __export only run once and iterate over all existing properties on exports, these bindings are never considered:

// @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 undefined in the emit, e.g. exports.foo = undefined; in the example above.

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 1, 2020
@sandersn sandersn added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Backlog Bug PRs that fix a backlog bug labels Feb 1, 2020
@weswigham
Copy link
Member Author

I've opened #36806 to review once this is in to address @ajafff 's concern about initializerless exported variables.

ping @rbuckton again for a review~

@ExE-Boss
Copy link
Contributor

I think we should try to use Object.prototype.__defineGetter__ if Object.defineProperty doesn’t exist, but __defineGetter__ does.

@weswigham
Copy link
Member Author

ping @rbuckton again again~

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

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

There are two new helpers which need to be added to tslib and also need to be added to the ExternalEmitHelpers enum and checked in checker.ts (see the call to checkExternalEmitHelpers at the bottom of checkExportDeclaration).

src/compiler/transformers/module/module.ts Show resolved Hide resolved
src/compiler/transformers/module/module.ts Show resolved Hide resolved
src/compiler/transformers/module/module.ts Outdated Show resolved Hide resolved
src/compiler/transformers/module/module.ts Show resolved Hide resolved
src/compiler/transformers/module/module.ts Outdated Show resolved Hide resolved
@weswigham weswigham merged commit 65e7acc into microsoft:master Feb 24, 2020
@weswigham weswigham deleted the live-bindings branch February 24, 2020 23:36
weswigham added a commit to weswigham/TypeScript that referenced this pull request Feb 25, 2020
oBusk added a commit to oBusk/walkable-buffer that referenced this pull request May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
8 participants