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

Emit real private methods #14781

Closed
zengfenfei opened this issue Mar 22, 2017 · 4 comments
Closed

Emit real private methods #14781

zengfenfei opened this issue Mar 22, 2017 · 4 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@zengfenfei
Copy link

In TS, private methods are inaccessible outside the defining class. But in emitted code, private methods are just ordinary JS methods which are accessible outside. I have a suggestion to emit real private methods for JS:

//*** TS code ***
class Greeter {
    greeting: string;
    constructor(message: string) {
        this.greeting = message;
        this.invisible();
    }
    private invisible() {
        console.log(this.greeting);
        this.greeting = 'new value';
    }
}

//*** Currently Emitted Code ***
var Greeter = (function () {
    function Greeter(message) {
        this.greeting = message;
        this.invisible();
    }
    Greeter.prototype.invisible = function () {
        console.log(this.greeting);
        this.greeting = 'new value';
    };
    return Greeter;
}());

//*** Suggested Emitted Code ***
var Greeter = (function () {
    function Greeter(message) {
        this.greeting = message;
        invisible(this);
    }
    function invisible(self) {
        console.log(self.greeting);
        self.greeting = 'new value';
    };
    return Greeter;
}());

Now the emitted private methods are inaccessible from outside. Another benefits is, self (the alias of this) and the private method name can be minified and even inlined by tools like uglyfyjs and google closure.

I've been using this kind of way to implement JS private methods in my projects. Per my experience, I haven't run into any side effects. And there's not performance implication either, here is a blog about this: http://andrewkelley.me/post/js-private-methods.html

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 22, 2017
@RyanCavanaugh
Copy link
Member

This would break any indirect uses of the method, break apply/call/bind, change the arity of arguments.length in unexpected ways, break use of arguments in the function body itself, and change the class prototype.

@zengfenfei
Copy link
Author

zengfenfei commented Mar 22, 2017

Yes, it will break the cases as you mentioned. So will you consider providing a compiler option, e.g. hidePrivateMethods? By default, it's false, but if some developers will take care of those cases and when it's safe to perform this optimization, they can set this option to true.

@RyanCavanaugh
Copy link
Member

We've gotten dozens of different "emit private things differently" requests and don't want to add flags for any of them (let's not even talk about how all of them would interact...). private is a compile-time construct only and the amount of care you need to actually completely hide something from outside observers is beyond what we could reasonably provide. This is doubly-so for ES6 emit, where private should simply evaporate and preserve all other semantics.

@kitsonk
Copy link
Contributor

kitsonk commented Mar 22, 2017

Dupe of #684, #8299, #13879 and discussed in the FAQ.

There are ways to construct things where the class can have a reference to a private WeakMap and not leak those on the instance, which is semi-sort-of how the TC39 proposal of how private fields would be implemented (actually it uses internal slots, but could be transpiled using a WeakMap).

It is also worth noting that it doesn't make any sense to change private and the emit at this point because likely TypeScript will want to/need to adopt the private fields proposal when it gets further down the path of being accepted. It is at Stage 2 and being actively worked on.

@mhegazy mhegazy closed this as completed Apr 21, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants