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

private fields are not private #564

Closed
reverofevil opened this issue Aug 29, 2014 · 14 comments
Closed

private fields are not private #564

reverofevil opened this issue Aug 29, 2014 · 14 comments
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead

Comments

@reverofevil
Copy link

Consider the following code

export class Test {
    private x: number;
    constructor() {
        this.x = 42;
    }
    f() : number {
        return this.x;
    }
}

Typescript generates the following JS

var Test = (function () {
    function Test() {
        this.x = 42;
    }
    Test.prototype.f = function () {
        return this.x;
    };
    return Test;
})();
exports.Test = Test;

Obviously, one could run something like new Test().x in a JS library, and private invariant would fail. I think it's not a kind of behaviour one would expect from private, at least in exported classes. The fix is quite simple:

var Test = (function () {
    var _private = {};
    function Test() {
        _private.x = 42;
    }
    Test.prototype.f = function () {
        return _private.x;
    };
    return Test;
})();
exports.Test = Test;
@NoelAbrahams
Copy link

The simple fix won't work:

``` JavaScript``
var Test = (function () {
var _private = { };
function Test(val) {
_private.x = val;
}
Test.prototype.f = function () {
return _private.x;
};
return Test;
})();

var x = new Test(50);
var y = new Test(100);

console.log(x.f()); // 100


This issue has been [extensively discussed on the old codeplex site.](https://typescript.codeplex.com/discussions/397651)

IMO the best solution for this is to obfuscate the private fields and members via minification #8.

@reverofevil
Copy link
Author

I've just tested a couple of (non-)solutions, read that thread, found most of them there and got demotivated. Sorry, guys. There's no way to make private variables private without messing the generated JS a lot, while it must be human-readable by language design.

@dthorpe
Copy link

dthorpe commented Sep 9, 2014

What about using Object.defineProperty() to define the private field with enumerable = false, so that the TS private fields are not "discoverable" to Javascript code? Yes, they'd still be accessible if the client code knew what property name to ask for (as the TS class itself would), but I think this would address the 90% case for keeping private fields out of circulation.

@RyanCavanaugh
Copy link
Member

We don't want to have runtime behavior change meaningfully between ES3 and ES5 compilation targets (Object.defineProperty is not available in ES3).

@NoelAbrahams
Copy link

@dthorpe,

I don't believe that using Object.defineProperty to set enumerable = false really solves anything. It only affects code in a for...in loop (as you do imply yourself).

The "discoverable" factor is already imposed by the IDE (VS for instance), which doesn't list private fields and methods in completion lists.

The generated code when using Object.defineProperty is a lot more verbose, which is a consideration for browser code.

@dthorpe
Copy link

dthorpe commented Sep 10, 2014

@NoelAbrahams The IDE does show the private fields when debugging JS code that uses the TS productions.

It just seems to me to be a really bad disconnect that the TS source declares X to be private when in fact it is openly visible and modifiable to the entire JS world.

The whole point of access control is to hide implementation details and protect internal state from outside manipulation. TS "private" does neither.

@RyanCavanaugh
Copy link
Member

The whole point of access control is to hide implementation details and protect internal state from outside manipulation

Many people would say the whole point of access control is to provide for cleaner layers of abstraction and reduce accidental use of other classes's implementation details. Enforcing 'private' as a compile-time construct accomplishes that goal.

It's basically impossible to write a JavaScript program that is resilient to malicious or intentional attempts to manipulate its behavior from within the same execution environment. Trying to solve that problem with 'private' is a non-starter.

@Hainesy
Copy link

Hainesy commented Jul 26, 2016

This philosophy is problematic mainly for the for ... in scenario. To get around it, I'm using the underscore prefix convention for all private fields, and then checking for property.startsWith("_") in the for ... in loop.

@acheshkov
Copy link

I can't understand why @RyanCavanaugh closed this issue !?

@RyanCavanaugh
Copy link
Member

Me neither, because I didn't?

@reverofevil
Copy link
Author

@acheshkov Let's imagine that TS compiled to assembly. Would private be still valid in assembly code? Of course, it wouldn't. But JS is essentially assembly language of the web. There is no guarantees for private fields to be still private in JS, and there shouldn't be any. Semi-readable JS code that compiler generates could make you think otherwise, but it's not true. That's why I closed my own issue.

@acheshkov
Copy link

Sorry guys,

i lost the main topic of this issue. My concern is not about encapsulation with private keyword. I just wanted to give my vote to the idea of using Object.defineProperty(..{enumerable: false} ...) for private properties.

@RyanCavanaugh the UX of github are confused looks like you closed it :)

@reverofevil
Copy link
Author

@acheshkov Object.defineProperty is not a very fast method. There's really no need to inflict slowdown on all programs with private fields, because if it matters to your code, you're using generated JS as if it were an implementation language, not assembly, and you're wrong.

@acheshkov
Copy link

@polkovnikov-ph

  • there is always a tradeoff between performance and language power/expressiveness. And my opinion that the second one is more important if we want to develop JS/TS.
  • i agree with you only in one that there are not right approach today to hide private methods from inherited instances even at runtime execution level. But actually it's possible and there are workarounds playing with Object.definePropery that can help in particular case.
  • about slow down. please look at https://jsperf.com/object-defineproperty-vs-definegetter-vs-normal. not so bad

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead
Projects
None yet
Development

No branches or pull requests

6 participants