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

Allow non-global helper functions #3364

Closed
vladima opened this issue Jun 3, 2015 · 17 comments
Closed

Allow non-global helper functions #3364

vladima opened this issue Jun 3, 2015 · 17 comments
Assignees
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@vladima
Copy link
Contributor

vladima commented Jun 3, 2015

Original issue: systemjs/builder#178

@pkozlowski-opensource
Copy link

@vladima is this issue is going to cover having some kind of runtime file with all the helpers? Currently using the noEmitHelpers: true flag will remove emitting helpers (like __decorate, __metadata etc.) from individual files. Still it would be great to have a file (as part of the TS npm distribution) that includes all those helpers.

Currently I'm hand-crafting this file on my end by moving generated helpers functions to one file checked-in in my project repo. Is there is a better way of doing this today? Or is it going to be covered by this issue?

@mhegazy
Copy link
Contributor

mhegazy commented Sep 30, 2015

Is this issue still needed?

@daniel-a-melo
Copy link

If helpers functions are emitted right before a System.register call it prevents this module from being loaded from SystemJS. As it seems to be a valid use case it would be nice to have some config option to prevent that

@mhegazy
Copy link
Contributor

mhegazy commented Oct 1, 2015

the helpers emit have been moved in 1.6 to inside the System.register call. so

export class B {}
export class C extends B {}

emits:

System.register([], function(exports_1) {
    var __extends = (this && this.__extends) || function (d, b) {
        for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
        function __() { this.constructor = d; }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    };
    var B, C;
    return {
        setters:[],
        execute: function() {
            B = (function () {
                function B() {
                }
                return B;
            })();
            exports_1("B", B);
            C = (function (_super) {
                __extends(C, _super);
                function C() {
                    _super.apply(this, arguments);
                }
                return C;
            })(B);
            exports_1("C", C);
        }
    }
});

@mhegazy
Copy link
Contributor

mhegazy commented Oct 1, 2015

I chatted with @vladima offline, this issue is tracking moving the helpers into a module, and alloing the compiler to emit imports to that module, e.g. the above code would emit:

System.register(["ts_helpers"], function(exports_1) {
    var __extends;
    var B, C;
    return {
        setters:[function(ts_helpers) {
            __extends = ts_helpers.__extends;
        }],
        execute: function() {
            B = (function () {
                function B() {
                }
                return B;
            })();
            exports_1("B", B);
            C = (function (_super) {
                __extends(C, _super);
                function C() {
                    _super.apply(this, arguments);
                }
                return C;
            })(B);
            exports_1("C", C);
        }
    }
});

@thorn0
Copy link

thorn0 commented Dec 15, 2015

As for now, where can I find a full list of all these helpers?

I use noEmitHelpers to substitute __awaiter with my own implementation, but also I must know what else I may break with this option.

@rbuckton
Copy link
Member

Right now, the only full list is in src/compiler/emitter.ts. We've been discussing releasing a separate script/library with all of our emit helpers.

If I may ask, what was the reason you chose to use your own implementation of __awaiter? Is there something deficient with the current implementation?

@thorn0
Copy link

thorn0 commented Dec 15, 2015

I'm experimenting with performance optimizations in Angular 1.x apps.

Angular has two implementations of promises with different behaviour: $q and $$q. They both are compatible with Promises/A+ and (mostly) with ES6. The difference is in how they schedule the execution of their callbacks. As for $q, the callbacks are executed during Angular's digest cycle. $$q works like normal ES6 promises. Basically, $$q is an optimization, which is used for async operations that shouldn't affect any Angular bindings, so there is no point in triggering the dirty checking (AKA digest) after they're done.

What I wanted to achieve is to avoid the unneeded converting of $$q promises to $q promises, which happened if I just did Promise = $q; and used the default implementation of __awaiter. So I replaced

function cast(value) {
  return value instanceof Promise && value.constructor === Promise ?
    value : new Promise(function (resolve) { resolve(value); });
}

with

function cast(value) {
  return value && typeof value.then === 'function' ? value : Promise.resolve(value);
}

instanceof doesn't work for Angular's promises (angular/angular.js#13545), so I resorted to such an unreliable (but good enough for me) check.

@rbuckton
Copy link
Member

IIRC, in Angular 1.x you get $q and $$q via dependency injection, and there is no Promise available globally or via a module import. You could always try:

// assumes $q and $$q are available in current lexical scope
type $q<T> = Promise<T>;
type $$q<T> = Promise<T>;
async function fn1(): $q<number> {
  await sleep(100);
  return 1;
}
async function fn2(): $$q<number> {
  await sleep(100);
  return 2;
}

For an async function, the return type annotation must be both a type and a value in the same scope. By creating a type alias for $q and $$q, you should be able to switch between them. I haven't tested this yet as I'm not terribly familiar with Angular 1.x.

@thorn0
Copy link

thorn0 commented Dec 16, 2015

Yes, this magic works. ✨ Is it documented?

That's how I initialize Promise, $q and $$q globals now:

var Promise: PromiseConstructor;
var $q: ng.IQService;
var $$q: ng.IQService;
type $q<T> = Promise<T>;
type $$q<T> = Promise<T>;
angular.module('app').run(['$q', '$$q', function($q_, $$q_) {
    Promise = $q_;
    $q = $q_;
    $$q = $$q_;
}]);

However, the default implementation of __awaiter still doesn't allow mixing different kinds of promises inside one async function without unneeded conversions. E.g., the result of the call to fn2 gets converted to $q here:

    async function fn3(): $q<string> {
        await fn1();
        await fn2();
        await fn1();
        return 'fn3';
    }

@Victorystick
Copy link

Ideally, transpiling something like the class below

export class A extends B {}

should yield something like this when targeting ES5 with ES6 modules.

import { __extends } from 'typescript-helpers';

export var A = (function (_super) {
    __extends(A, _super);
    function A() {
        _super.apply(this, arguments);
    }
    return A;
})(B);

Targeting CommonJS modules might yield something like

var typescriptHelpers = require('typescript-helpers');

var A = (function (_super) {
   typescriptHelpers. __extends(A, _super);
    function A() {
        _super.apply(this, arguments);
    }
    return A;
})(B);

exports.A = A;

How difficult would this be to implement?

@evmar
Copy link
Contributor

evmar commented Feb 18, 2016

When you pass TypeScript code through the Closure compiler, the underscore prefix on these names prevents Closure renaming of the function names. I guess if they're pulled from a namespaced module you'd no longer need the underscores.

(Edit: moved this into issue #7345.)

@AdamWillden
Copy link

I really like the idea @Victorystick proposed. That would be a really neat way to handle it.

In my case I do want the helpers, I just don't want them respecified in every file they're needed. I'm sure I'm one of many with that desire.

We can then treat the 'typescript-helpers' as any other library, and when bundling, with SystemJS for example, it'll automatically pick out the files required for the bundle to function. The idea of managing the helpers ourselves isn't great for maintainability.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 15, 2016

Should be addressed by #9097. a new flag --importHelpers allows for importing helpers from a standalone runtime library (still to be published on npm), and the compiler will emit imports to this library whenever a helper is needed.

@mhegazy mhegazy added Suggestion An idea for TypeScript Fixed A PR has been merged for this issue and removed Bug A bug in TypeScript labels Jun 15, 2016
@ghost
Copy link

ghost commented Jun 30, 2016

This is marked with Milestone 2.1, but on the Roadmap it's under v. 2.0.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 30, 2016

updated the roadmap. thanks.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 20, 2016

This should be in typescript@next already. New --importHelpers results in importing emit helpers from "tslib". See #9097 (comment) for more details.

Users are expected to include tslib in their production bits. i.e. as a node dependency for node packages, or as a script tag for html applications.

tslib is hosted at https://github.com/Microsoft/tslib; and available at npm install tslib. please see https://github.com/Microsoft/tslib for more information.

@mhegazy mhegazy closed this as completed Sep 20, 2016
enlight added a commit to debugworkbench/hydragon that referenced this issue Sep 29, 2016
This prevents helper functions such as __awaiter, __decorate, etc. from
being emitted into each and every file that needs them (resulting in
multiple copies of the same helper functions all over the code base),
and imports them from the new `tslib` module instead.

More details at microsoft/TypeScript#3364 (comment)
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

10 participants