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

Change the emit for __extends to support easy global override #1622

Closed
basarat opened this issue Jan 9, 2015 · 7 comments
Closed

Change the emit for __extends to support easy global override #1622

basarat opened this issue Jan 9, 2015 · 7 comments
Assignees
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@basarat
Copy link
Contributor

basarat commented Jan 9, 2015

Change the emit for __extends to be :

var __root = typeof global !== "undefined" && global || typeof window !== "undefined" && window || this;
var __extends = __root.__extends = __root.__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 __();
}

Reason:

It will allow you to override __extends globally with a single require call to a file that contains the following:

var __root = typeof global !== "undefined" && global || typeof window !== "undefined" && window || this;
__root.__extends = function(d,b){/*custom __extends implementation*/}

Note: all AMD / CommonJS / internal modules will work

More : #1601 (comment)

@Arnavion
Copy link
Contributor

Arnavion commented Jan 9, 2015

(Copying the comment from #1601 here)

  • this, self and window in a script loaded in a browser refers to the global object.
  • this on node can either be the global object (when a file or script is run as the main module) or a module object (when a file is require'd). However global refers to the global object in both cases.
  • Because this is a module-specific object in a require'd piece of code, it is not possible for a require'd module to override the TS-emitted __extends implementation in the module it's being require'd into. It has to be necessarily overriden in the same file that contains it, and this has to be done in every file that has the emit.

This new emit tries to find a few well-known globals (node and browser) and falls back to this if it can't. It checks for and attaches the __extends function to this object. This way, a module can be written to override __extends (like the first block in that comment), and any module which wishes to use it can simply require() it.

The only user-visible difference from the current emit that I can think of is that, if the user is on node and already has a different __extends registered on the global object, it will now be clobbered. However users should not expect names prefixed with two underscores to be available to them, so I think this is acceptable. Also, if they already have a variable named __root in scope it will be clobbered. The same reasoning applies there. TS could complain about __root being used by user code, like it complains about user-defined _this, _i, etc. (TS2397-2402).

Alternatively, it could be done in an IIFE that only exposes __extends:

var __extends = (function (root) {
    return root.__extends = root.__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 __();
    };
})(typeof global !== "undefined" && global || typeof window !== "undefined" && window || this);

Then no new variable is introduced and no new diagnostic is needed from TS.

I think this is not much more complicated than the current __extends. In particular, it should have no difference from the current emit in terms of performance, because the actual __extends implementation is still the same - it does not hold on to anything new from its closure.

For users who are already overridding __extends with their own var __extends declaration in the same file (both browser and node), this will continue to clobber the TS-emitted function. For users who override it in browser JS in a separate <script> block (either with var or by assigning to window.__extends), this will still continue to work.

@danquirk danquirk added the Suggestion An idea for TypeScript label Jan 14, 2015
@pspeter3
Copy link

Another option seems to be generating an extends module and having it be imported by all modules that use extends

@whitneyit
Copy link
Contributor

@pspeter3 +1 Similar to the issue I raised in #1350

@rbuckton
Copy link
Member

We are considering the following change, which would only be observably different in CommonJS (details below):

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 __();
};

Here are some additional details on this proposal:

  1. We would not emit a var for __extends. This means that a variable declaration for __extends will not be hoisted in the current scope and will force __extends to refer to the global scope.
  2. If a global __extends exists, it will not be overwritten.
  3. There is no need to capture a direct reference to the global environment (via window, self, global, or this), so this should work on all platforms (Web Browser, Web Worker, nodejs, iojs, cscript, etc.).

In general, this would not be observably different for scripts or --module amd. However, there would be an observable change to --module commonjs. The current approach that uses this has unspecified behavior in CommonJS, based on the execution context as @Arnavion mentioned above. This change would bring the CommonJS runtime behavior in line with the AMD behavior. In addition, this change would help with some of our other helpers we are emitting for decorators in ES6, where this is guaranteed to be undefined in a module body.

The downside is that this change would introduce a global identifier in CommonJS (and also eventually in ES6 modules) that would not have previously been defined. Also, running a linter against the output may result in a warning or error due to introducing a global identifier.

We would like to get some feedback on this approach, and are considering this change for 1.5 beta. If you are aware of any existing behavior that would be negatively impacted by this change, please let us know.

@Arnavion
Copy link
Contributor

/cc @csnover

@danquirk
Copy link
Member

As recently noted, does #2901 cover everything folks wanted here?

@mhegazy mhegazy added the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label Dec 9, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Feb 22, 2016

#2901 should address the underlying issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants