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

Add __createBinding and __setModuleDefault helpers #89

Closed
wants to merge 4 commits into from

Conversation

weswigham
Copy link
Member

This is the tslib PR for microsoft/TypeScript#35967

@weswigham
Copy link
Member Author

cc @rbuckton do you have any feedback here, too?

tslib.js Outdated Show resolved Hide resolved
tslib.js Outdated Show resolved Hide resolved
tslib.js Outdated
@@ -273,4 +295,6 @@ var __classPrivateFieldSet;
exporter("__importDefault", __importDefault);
exporter("__classPrivateFieldGet", __classPrivateFieldGet);
exporter("__classPrivateFieldSet", __classPrivateFieldSet);
exporter("__createBinding", __createBinding);
exporter("__setModuleDefault", __setModuleDefault)
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.

I'm not sure this actually needs to be exposed, since no TypeScript code will emit tslib_1.__setModuleDefault (since its only used by __importStar).

tslib.es6.js Outdated Show resolved Hide resolved
tslib.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ExE-Boss ExE-Boss left a comment

Choose a reason for hiding this comment

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

You forgot to remove __setModuleDefault from one more location.

@@ -35,6 +35,8 @@ var __importStar;
var __importDefault;
var __classPrivateFieldGet;
var __classPrivateFieldSet;
var __createBinding;
var __setModuleDefault;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var __setModuleDefault;

o[k2] = m[k];
});

__setModuleDefault = Object.create ? (function(o, v) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
__setModuleDefault = Object.create ? (function(o, v) {
var __setModuleDefault = Object.create ? (function(o, v) {

@@ -34,3 +34,4 @@ export declare function __importStar<T>(mod: T): T;
export declare function __importDefault<T>(mod: T): T | { default: T };
export declare function __classPrivateFieldGet<T extends object, V>(receiver: T, privateMap: WeakMap<T, V>): V;
export declare function __classPrivateFieldSet<T extends object, V>(receiver: T, privateMap: WeakMap<T, V>, value: V): V;
export declare function __createBinding(t: any, mod: any, k: string, k2?: string): void;
Copy link
Contributor

@ExE-Boss ExE-Boss Feb 25, 2020

Choose a reason for hiding this comment

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

This should probably be:

Suggested change
export declare function __createBinding(t: any, mod: any, k: string, k2?: string): void;
export declare function __createBinding(t: object, mod: object, k: string, k2?: string): void;

or

Suggested change
export declare function __createBinding(t: any, mod: any, k: string, k2?: string): void;
export declare function __createBinding(t: object, mod: object, k: PropertyKey, k2?: PropertyKey): void;

@@ -104,7 +104,7 @@ export function __generator(thisArg, body) {
}

export function __exportStar(m, exports) {
for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
for (var p in m) if (!exports.hasOwnProperty(p)) __createBinding(exports, m, p);
Copy link
Contributor

Choose a reason for hiding this comment

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

exports.hasOwnProperty(p) won’t work in case exports has a null prototype or has overridden hasOwnProperty(…): microsoft/TypeScript#37013 and #92.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably get in the habit of saving off intrinsics like this (i.e. var hasOwn = Object.prototype.hasOwnProperty and hasOwn.call(exports, p)), but we don't necessarily want to have to introduce a __hasOwn helper for that kind of case.

That said, its up to @weswigham if he wants to take on that change for tslib right now, since it's out of scope for this change.

Copy link
Member

Choose a reason for hiding this comment

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

That said, we do call Object.hasOwnProperty.call in __importStar below this.

@weswigham
Copy link
Member Author

You forgot to remove __setModuleDefault from one more location.

Mmmm yes and no - the helper TS emits without importHelpers for __setModuleDefault checks for this.__setModuleDefault; so this makes sure that we use the tslib version if it's available. So it's not exported, per sey, as an intended entrypoint, but it is intentionally global in contexts where that makes sense, I guess?

@rbuckton
Copy link
Member

the helper TS emits without importHelpers for __setModuleDefault checks for this.__setModuleDefault

We could always change that to the way that we define extendsStatics for the __extends helper. Either way, I'm not sure how important it is that we expose this one as a global.

@weswigham
Copy link
Member Author

#99 now includes this, plus feedback~

@weswigham weswigham deleted the create-binding branch May 6, 2020 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants