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

[Transforms] Import external helpers module #9097

Merged
merged 10 commits into from
Jun 15, 2016

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Jun 11, 2016

This change adds support for referencing emit helpers from an external module.

Compiler Options

  • --importHelpers - Import emit helpers from "tslib".

Emit

When importHelpers is specified, a synthetic ImportDeclaration is added to the top of each external module that needs an emit helper, such as __extends. As a result, no emit helpers will be added to these files during emit. In addition, any place where the emit helper would be referenced, it would instead reference a member of the imported module.

For example:

// @module: commonjs
// @importHelpers: true
export class A { }
export class B extends A { }

Outputs:

var tslib_1 = require("tslib");
var A = (function () {
  function A() {
  }
  return A;
})();
exports.A = A;
var B = (function (_super) {
  tslib_1.__extends(B, _super);
  function B() {
  }
  return B;
})(A);
exports.B = B;

Script files

If a file is detected as a non-external-module (e.g. script file), the existing behavior for emit helpers is used. As long as the --noEmitHelpers option is not set, emit helpers will be added to the top of the file as usual.

Fixes #3364

@rbuckton rbuckton added the Domain: Transforms Relates to the public transform API label Jun 11, 2016
@rbuckton
Copy link
Member Author

rbuckton commented Jun 13, 2016

CC: @mhegazy, @vladima, @ahejlsberg

@@ -415,7 +415,7 @@ namespace ts {
BlockScoped = Let | Const,

ReachabilityCheckFlags = HasImplicitReturn | HasExplicitReturn,
EmitHelperFlags = HasClassExtends | HasDecorators | HasParamDecorators | HasAsyncFunctions,
EmitHelperFlags = HasClassExtends | HasDecorators | HasParamDecorators | HasAsyncFunctions | HasJsxSpreadAttribute,
Copy link
Contributor

Choose a reason for hiding this comment

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

side note, we should get rid of this flag, specially that it is not used except in one place, and is at bit 30, which will cause a conversion on node-32 bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this is a bug in printer. This flag needs to be used and isn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, we need to consolidate the node flag values now that I've extracted the modifiers out of NodeFlags, that should help us avoid the SMI overflow on 32-bit node.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 14, 2016

__asing seems to be emitted still.

other than that, can you add a test for something that explicitly includes an import to tslib.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 14, 2016

We previously talked about adding a check in the program creating to make sure that tslib can be resolved as a module. and possibly, that it does have an exported function __extends, etc..

if (helpers) {
const exports = helpers.exports;
if (requestedHelpers & NodeFlags.HasClassExtends && languageVersion < ScriptTarget.ES6) {
verifyHelperSymbol(exports, "__extends", SymbolFlags.Value);
Copy link
Contributor

Choose a reason for hiding this comment

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

consider returning after the first error, to avoid reporting the same error for every file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to traverse each file to determine which helpers I need to verify. I can possibly add some additional short-circuiting logic, but nothing as simple as "bail on first error".

@mhegazy
Copy link
Contributor

mhegazy commented Jun 15, 2016

Baselines need to be updated for error spans i would expect.

@rbuckton rbuckton changed the title [Transforms] Transforms import helpers [Transforms] Import external helpers module Jun 15, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Jun 15, 2016

👍

@rbuckton rbuckton merged commit a766005 into transforms Jun 15, 2016
@rbuckton rbuckton deleted the transforms-importHelpers branch June 15, 2016 18:39
@develar
Copy link

develar commented Jun 16, 2016

Will be useful to allow specify own module name. In my project https://github.com/electron-userland/electron-builder/blob/master/src/codeSign.ts I need only awaiter and since bluebird is forbidden, I disable emit helpers and add custom awaiter impl to each file. And new option is not solution because I cannot specify helper module name.

@gilboa23
Copy link

Will the new --importHelpers flag also apply to the __export helper? Currently, the --noEmitHelpers controls all the other helpers (e.g., __extends, __decorate, etc.), except for __export. See issue #10905.

@rbuckton
Copy link
Member Author

@gilboa23, no it doesn't yet apply to __export as that helper is currently a closure over the local exports symbol defined in the module body. We are considering a change to make this possible, but it would be a breaking change for anyone using --noEmitHelpers.

@gilboa23
Copy link

Yes, but I was not referring to the existing --noEmitHelpers flag. What I meant was to apply this change just over the new --importHelpers flag by passing the local exports symbol in the invocations of the imported tslib.__export() function. This should not be a breaking change since this is a new flag that no one is using yet. Something like:

// @module: amd
// @importHelpers: true
export * from "A";
export * from "B";

Output:

define(["require", "exports", "A", "B"], function (require, exports, A_1, B_1) {
    "use strict";
   var tslib_1 = require("tslib");
    tslib_1.__export(A_1, exports);
    tslib_1.__export(B_1, exports);
});

@mhegazy
Copy link
Contributor

mhegazy commented Oct 15, 2016

@gilboa23 if issue #10905 is fixed, it will work for both.

eddiegroves added a commit to eddiegroves/eddie-template that referenced this pull request Oct 20, 2016
Allows ES5 helpers such as _extends to be placed into one external
source file, rather than inlined into every module.

Also removes ts-loader in favour of experimenting with just tsc.

See: microsoft/TypeScript#9097
@wyntau
Copy link

wyntau commented Nov 14, 2016

[email protected] is missing __generator function. But the source code has.

should tslib on npm be updated and release a new version?

@itaysabato
Copy link

So, if I would like to use --importHelpers with tslib and replace only a specific helper (say, __awaiter) with my own implementation, how would I go about that?

@mhegazy
Copy link
Contributor

mhegazy commented Dec 23, 2016

So, if I would like to use --importHelpers with tslib and replace only a specific helper (say, __awaiter) with my own implementation, how would I go about that?

  1. Use [email protected] or later
  2. Install tslib
  3. Set --importHelpers on the command line or in your tsconfig.json

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: Transforms Relates to the public transform API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants