Add support for --noEmitHelpers flag

[whitneyit]

This PR is a Work In Progress that addresses multiple __extends
being output as described in #1350: Multiple __extends being output
when --module amd is set.

The issue still exists as of v1.5.0 - f53e6a8.

Apparently a fix was created for this in #1356 but according to #2009, a
comment later indicated that this was never merged in.

Further conversation continued in #2487 but did not yield any result. I
refer to my earlier recommendation in #1350.

My question is this, would the TypeScript team be open to a flag that
can be passed to tsc that will generate something like the following

define(["require", "exports", "__extends", './mammal'], function (require, exports, __extends, Mammal) {
    var Human = (function (_super) {
        __extends(Human, _super);
        function Human() {
            _super.apply(this, arguments);
        }
        return Human;
    })(Mammal);
    return Human;
});

To continue with the naming convention I have chosen the flag
--noEmitHelpers.

Edit: renamed noEmitExtends to noEmitHelpers

[msftclas]

Hi @whitneyit, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[mhegazy]

tagging @rbuckton, he is working on a proposal for this.

I would not call it extends, we now have more helpers than extends, and possibly more, and would not want to add a --noEmit flag. so i would say call it --noHelpers, --noHelperEmit, or helpers: none vs helpers:embed*,

I am bad with naming things in general, so suggestions appreciated.

[mhegazy]

why not just assume a global will exist? and do you expect a module called __extends will exist?

[mhegazy]

what about other helpers?

[whitneyit]

Because this is targeting amd modules, as stated in #1350, the following module could be defined.

define('__extends', function () {
  return function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
     __.prototype = b.prototype;
    d.prototype = new __();
  };
});

[whitneyit]

This could then be defined and included by the user however they see fit. The TypeScript documentation could be updated to include this boilerplate if this flag gets greenlit.

[whitneyit]

I had a thought about that and the issue I ran into was what to do about __decorate, __metadata, and __param?

I was going to add an Array<string> type to src/compiler/commandLineParser.ts so that individuals could chose to opt out of that TypeScript generated code, but that seemed a little premature.

To clarify, the Array<string> could be used as you said for a flag like --noHelpers extends,param,decorate or something like that

[mhegazy]

I would argue that the group of users who want to control one heler would likely want to control all or at least would not mind if they are asked to. Hence my proposal of a single switch.

[whitneyit]

I agree. If you think it is within scope of the issue at hand I'll update the PR to --noEmitHelpers and make that work across the board

[mhegazy]

I would want to get @rbuckton's input on this as well.

[whitneyit]

I've renamed the variable, I'll await @rbuckton's input

[CyrusNajmabadi]

If we don't emit the helper, then what happens when __extends is called? If it's that an existing __extends will be invoked, then perhaps we need to change our declaration of __extends to be more like:

var __extends = __extends || ...

So that we'll defer to your __extends if you provide it.

[whitneyit]

No existing helper will be called. It will be up to the developer to implement their own version. The use case for this is pretty much isolated to amd/umd modules due to the way the helper gets emitted over numerous files

[whitneyit]

A comment was made on a line diff that is now outdated. It shows how a developer would define their own __extends function specifically for this use case. This also will have the benefit of then being able to potentially score a 100% on code coverage

define('__extends', function () {
  return function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
     __.prototype = b.prototype;
    d.prototype = new __();
  };
});

[rbuckton]

We are considering a different approach to making __extends and other helpers available with something like --noEmitHelpers, so I would recommend removing this line.

[whitneyit]

Sorry @rbuckton I'm a little confused by that comment. Does this still apply? The intention behind this line is to inject an implementation of said helper that user can tap into.

I'm thinking the check still needs to be there but maybe some logic could be applied to only add in the helper module when needed. Do you agree?

[mhegazy]

This makes it required for users to define "__extends" as a module, only in AMD., but global otherwise. i think we need to make it always global. i.e. remove the addition of "__extends" as a dependency name. If you want to override that you need to define a global var __extends:

something like:

if (typeof __extends !== "function") {
  __extends = function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
     __.prototype = b.prototype;
    d.prototype = new __();
  };
}

[rbuckton]

@whitneyit If you look at #2911, what we are considering is making a separate runtime library available that contains these helpers, which would allow you to write the following:

tsc --noEmitHelpers --import tslib --module commonjs app.ts

So your app.js output might be the following for CommonJS:

require("tslib");
// app body

Or the following for AMD:

define(["require", "exports", "tslib"], function (require, exports) {
  // app body
});

And the tslib library (or whatever it is called), would define globals for __extends, __decorate, etc.

[whitneyit]

If the tslib runtime is going to handle these helpers is there anything from with say using: tslib.extends instead of __extends? That way we could do away with globals in all contexts/module patterns

[rbuckton]

Currently we have no way of knowing that we should import __extends from a library. We also need to be able to use these globals for <script/> references in the browser. That said, while the approach we're considering does introduce globals, you can still write the following, as the helpers are exported as well:

import { __extends } from 'tslib';

[whitneyit]

But you would know if a --module argument has been passed wouldn't you? That's what I was trying to do with the above line:

write(", \"__extends\"");

I was trying to add __extends as a module to the amd pattern. So if we are able to deduce that we are in a module then couldn't we use the tslib.extends pattern and use __extends otherwise. How's that sound?

Example taken from #1350

// commonjs
var __extends = require('./tslib').extends;
var Mammal = require('./mammal');
var Human = (function (_super) {
    __extends(Human, _super);
    function Human() {
        _super.apply(this, arguments);
    }
    return Human;
})(Mammal);

// amd
define(["require", "exports", "tslib", './mammal'], function (require, exports, tslib, Mammal) {
    var Human = (function (_super) {
        tslib.extends(Human, _super);
        function Human() {
            _super.apply(this, arguments);
        }
        return Human;
    })(Mammal);
    return Human;
});

// script
var __extends = this.__extends || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    __.prototype = b.prototype;
    d.prototype = new __();
};
var Animal = (function () {
    function Animal() {
    }
    return Animal;
})();
var Mammal = (function (_super) {
    __extends(Mammal, _super);
    function Mammal() {
        _super.apply(this, arguments);
    }
    return Mammal;
})(Animal);
var Human = (function (_super) {
    __extends(Human, _super);
    function Human() {
        _super.apply(this, arguments);
    }
    return Human;
})(Mammal);

[whitneyit]

@mhegazy, in regards to your comment on #2911, would you want to implement the --noEmitHelpers flag as an Array<string> in src/compiler/commandLineParser.ts?

I was under the impression from your earlier comments that this would not be the case

wrap this whole section with if (!compilerOptions.noEmitHelpers) check.

Unless we want to have the user specify which helper they do/don't want.

[mhegazy]

thanks @whitneyit. In addition to my comment about removing the "__extends" from the AMD module header, we will need a test for the new flag. to add the test, you need to:

• add a new test file in test\cases\compiler.ts with a comment at the top of // @noEmitHelpers: true
• add handeling for the new switch in harnes.ts
  and in fileMetadataNames
• jake runtests
• jake baseline-accept

[whitneyit]

@mhegazy your above comment makes a lot of sense. I'll start making the appropriate tests and update the PR

[whitneyit]

I've taken my first pass at adding a test.Once we can determine a reasonable implementation for all the use cases amd, commonjs, umd, <script> I will add additional tests to cover.

I believe that there is still some discussion on whether @rbuckton's comment about tslib should be factored into this PR.

[mhegazy]

thanks @whitneyit, i have manually merged the change. We need a different change to manage adding a way to include polifills as well as helpers. we also need another repo for tslib, to get the basic __extends and __decorat..etc.

[mhegazy]

@rbuckton please update this with your issue for including pollifills.

[whitneyit]

Thanks for the merge! I'll look out for the upcoming PR and help where I can 😄

[whitneyit]

It doesn't look like it was referenced here but #1622 seems relevant