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

Explain why private and protected members in a class affect their compatibility #18499

Open
jandsu opened this issue Sep 15, 2017 · 28 comments
Open
Labels
Docs The issue relates to how you learn TypeScript

Comments

@jandsu
Copy link

jandsu commented Sep 15, 2017

TypeScript Version: up to 2.5.2 at least

In the doc a short paragraph explains that private and protected members in a class affect their compatibility.

I have been searching for a while in the design goals, on SO etc... but could not find a decent explanation of the rationale. Could we:

  • either document the rationale (in the paragraph above?
  • or consider evolving the language?

Note: I found one rationale close to what I am looking for, but you could imagine that the language "reserves" the private names but does not compel to use them.

Code
This behavior is especially an issue in unit testing where you want to mock a class, disregarding its internals...

class MyClass {
    pubfield = 0;
    private privfield1 = 0;
    constructor(private privfield2: number) {}
    private method(): void {}
}

class MyClassMock implements MyClass {
    pubfield: number;
    // *** compile errors on missing properties ***
}

Expected behavior:
I would like to only care about the public contract that a class-under-test can possibly use from my mock.

Actual behavior:
I cannot limit my mock to the public fields.

The ugly workaround I found is the following:

class MyClass {
    pubfield = 0;
    private privfield1? = 0;
    private privfield2? = 0;
    // do not use the shortcut notation because the constructor param is not optional!
    constructor(privfield2: number) {
        this.privfield2 = privfield2;
    }
    private method?(): void {}
}

class MyClassMock implements MyClass {
    pubfield: number;
}

What I do not like is that I have to modify the semantics of my class to be able to mock it and that I am scared that a future overzealous refactoring may use the shortcut notation for fields declaration in the constructor... making the constructor param optional (!)

@kitsonk
Copy link
Contributor

kitsonk commented Sep 15, 2017

Because, at the moment, in ECMAScript, there is no real private or protected, therefore they affect the shape of the object, and therefore it is safer to consider them for assignability purposes. This keeps you from accidentally overwriting private or protected members.

Consider the following:

class Foo {
  private _bar = 'bar';

  log() {
    console.log(this._bar);
  }
}

class Baz extends Foo {
  private _bar = 'baz';
}

const baz = new Baz();
baz.log(); // if TypeScript allowed it, it would log "baz" which is not what it should be

Because all properties and methods are visible at run-time, TypeScript has no choice but to consider those as part of the shape/structure of an instance.

@jandsu
Copy link
Author

jandsu commented Sep 15, 2017

Hello @kitsonk and thank you for this quick response! Up to here, this is what I had understood. Nevertheless...

I would understand that I am not allowed to overwrite a private/protected name, but why force me to implement it when all I want is to implement the same contract (as in unit test stubs).

This is exactly the behavior of my workaround using the all-privates-are-optional--patent-pending pattern:

class MyClass {
    pubfield = 0;
    private privfield1? = 0;
    private privfield2? = 0;
    constructor(privfield2: number) {
        this.privfield2 = privfield2;
    }
    private method?(): void {}
}

class MyClassMock implements MyClass {
    pubfield: number;
    private privfield1? = 0;
    // ** error ** types have separate declarations of a private property 'privfield1'
}

const foo: MyClass = {
    pubfield: 1,
    privfield1: 2 // ** error ** Property 'privfield1' is private in type 'MyClass'...
};

I come from years of rigid typing systems and the refreshing side of languages such as Javascript and Python is this "duck typing" which makes mocking a pleasure (but brings other scary issues).

To me Typescript is bringing the best of both worlds... in a kind of magical balance... Here we seem to be losing our balance IMHO.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 15, 2017

It is dealing with the reality that there are no private or projected members. TypeScript could bury its head in the sand and ignore it, or it could face the reality and protect its users. It isn't a TypeScript thing, it is ECMAScript thing. ALL properties are public at run-time, therefore they are part of the structure, therefore in a structural typing system they count as part of the structure.

In certain use cases it is much better to create truly private data, using the language:

interface PrivateData {
  foo?: string;
}

const privateData = new WeakMap<Foo, PrivateData>();

class Foo {
  constructor() {
    privateData.set(this, {});
  }

  log() {
    console.log(privateData.get(this).foo));
  }
}

export default Foo;

In this situation, the private data is truly private and does not effect the structure of the instance.

The issue to implement the ECMAScript proposal for private fields is tracked at #9950. One can only assume once there are private fields that are truly private, they will not be considered as part of the structure of the instance.

@jandsu
Copy link
Author

jandsu commented Sep 15, 2017

Well, again this is a matter of balance... why have implemented the private / protected keywords in Typescript if the suggestion is not to use them and to come up with a pattern such as the one you suggest?

This pattern is fine to store sensitive data such as an authentication token (to prevent access by a browser extension for example), but you will probably admit that is overkill for the standard usage of the private fields like internal state flag etc...

Those two keywords are in the language now and the question is whether they are used to make the most sense out of them or not... When I read the language design goals I have the feeling that the current behavior is trying to be too "sound"...

Indeed, even within Typescript code, you can always call Object.getPrototypeOf() and bypass compiler visibility very easily... so trying to prevent mistakes should be preferable to prevent real access.

Anyway, I will not bother you any longer with an endless discussion... Either you got my point or at least it just feeds your thoughts (the mocking use case in particular). I already apologize for being so insistent...

Whatever the outcome, I guess this point is surprising enough that it deserves a few more lines in the documentation... and this does not change the fact that you guys came to a wonderful balance with this language 🥇

@RyanCavanaugh RyanCavanaugh added the Docs The issue relates to how you learn TypeScript label Sep 15, 2017
@RyanCavanaugh
Copy link
Member

Allowing the private fields to be missing would be an enormous problem, not some trivial soundness issue.

Consider this code:

class Identity {
  private id: string = "secret agent";
  public sameAs(other: Identity) {
    return this.id.toLowerCase() === other.id.toLowerCase();
  }
}

class MockIdentity implements Identity {
  public sameAs(other: Identity) { return false; }
}

MockIdentity is a public-compatible version of Identity but attempting to use it as one will crash in sameAs when a non-mocked copy interacts with a mocked copy.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 17, 2017

First to be clear I am not a member of the core team. Some of the comments felt as if you were addressing someone who actually makes decisions.

why have implemented the private / protected keywords in Typescript if the suggestion is not to use them and to come up with a pattern such as the one you suggest?

I suspect, with the ES private members at the stage they are at, the TypeScript team would simply not have implemented them. But classes in TypeScript were implemented several years ago, even when classes in ECMAScript weren't even a certain thing. Private and protected where strong concepts from other classed based languages and were often implemented by convention in JavaScript at that point, usually denoted by an _. This choice gave some type safety for these conventions without changing the run-time behaviour of the code.

This pattern is fine to store sensitive data such as an authentication token (to prevent access by a browser extension for example), but you will probably admit that is overkill for the standard usage of the private fields like internal state flag etc...

Agreed, but it feels like you think it is an "opinion" if private and protected members are optional to be considered as part of the structural shape of the object. As Ryan pointed out, it really isn't an opinion. They effect the structural shape and it is more sound for TypeScript to consider them. I don't think it is an opinion, or striking a balance. It is more of an abject fact.

@jandsu
Copy link
Author

jandsu commented Sep 18, 2017

Hello guys,
Thanks for your answers and your time answering my question.
Indeed the code in Ryan's example is definitely a good reason why it is the way it is. This is exactly the reason why I was asking either for a documentation improvement or... for a possible update in the language.
I hope that this doc update will help other developers understand the rationale for this decision when they will be learning Typescript.
Best regards

PS : my pattern of optional private fields is after all not so silly: making Identity.id optional forces the developer to consider that case if required

@kitsonk
Copy link
Contributor

kitsonk commented Sep 18, 2017

PS : my pattern of optional private fields is after all not so silly: making Identity.id optional forces the developer to consider that case if required

Side note, if during the lifecycle of a class, a private field might be undefined, I prefer to utilise | undefined versus the optional flag. In interfaces it sort of makes sense, as you are describing what the shape of an object looks like, but in instantiatable classes, that sort of Schrödinger's cat property seems less appropriate.

@jandsu
Copy link
Author

jandsu commented Sep 21, 2017

For those who might reach this issue through a search on mocking issues, here I like better pattern than my private-is-optional one described above:

class MyClass {
    pubfield = 0;
    private privfield1 = 0;
    constructor(private privfield2: number) {}
    private method(): void {}
}

class MyClassMock /* does not implement MyClass this time! */ {
    pubfield: number;
}

const foo = new ClassUnderTest(new MyClassMock() as any as MyClass);

This is the kind of weird believe me! cast, but at least you can do unit testing w/o putting your private fields optional.

@unional
Copy link
Contributor

unional commented Nov 13, 2017

A trick to quickly implement all the public interfaces:

class MyClassMock implements MyClass {
}

Use the IDE (VSCode) auto implement feature to implement the methods:

class MyClassMock implements MyClass {
  pubField: number
}

And then removing the interface:

class MyClassMock {
  pubField: number
}

Repeat that whenever you have new public properties introduced in MyClass.

@unional
Copy link
Contributor

unional commented Nov 13, 2017

class Identity {
  private id: string = "secret agent";
  public sameAs(other: Identity) {
    return this.id.toLowerCase() === other.id.toLowerCase();
  }
}

class MockIdentity implements Identity {
  public sameAs(other: Identity) { return false; }
}

Regarding this, I'm not sure if this is a good example. This seems to me that it pushes the C# feature to TS a bit too far.
If we consider "true private" in JS, the other.id is not accessible.
So it is not really a valid JS.

@unional
Copy link
Contributor

unional commented Nov 13, 2017

@jandsu regarding const foo = new ClassUnderTest(new MyClassMock() as any as MyClass);

You can simply do: const foo = new ClassUnderTest(new MyClassMock() as any);

You don't have to cast it back to MyClass because TS is duck typed.
The ClassUnderTest implementation will assume the input is an instance of MyClass, and you don't have to "test" the assignability.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Nov 13, 2017

Regarding this, I'm not sure if this is a good example. This seems to me that it pushes the C# feature to TS a bit too far. If we consider "true private" in JS, the other.id is not accessible.

This is hardly a C#ism. C++, Java, Swift, etc, all allow cross-instance private property access. See #10516 for discussion on this

@unional
Copy link
Contributor

unional commented Nov 13, 2017

This is hardly a C#ism. C++, Java, Swift, Ruby, etc,

Ok, agree that it is not C#ism. 🌷
However, it is still not how JS behaves.
Probably in this regards JS is superior than all other languages. 😆

btw, nothing against C#, I came from C#. :)

@RyanCavanaugh
Copy link
Member

In JS, all object properties are public 🤔 😉

@kitsonk
Copy link
Contributor

kitsonk commented Nov 14, 2017

However, it is still not how JS behaves.

As stated, all object properties are public and in the fields proposal private fields will also be accessible across instance (though it is only alluded to, but it will follow patterns of other languages). Even if you create a private via a WeakMap (which is the way private fields will be polyfilled) other instances will inherently be able to read the private fields of other instances.

What part of JavaScript are you referring to?

@unional
Copy link
Contributor

unional commented Nov 14, 2017

What part of JavaScript are you referring to

closure

function constructor() {
  var x = 1
  return {
    getX() {
      return x;
    }
  }
}

@kitsonk
Copy link
Contributor

kitsonk commented Nov 14, 2017

You get a point for showing that JavaScript can solve things in various ways, but it is still arguable how JavaScript works. This was the more intentional pattern as part of the language prior to ES6:

function Foo() { }

Foo.prototype = {
  _x: 1,
  getX: function () {
    return this._x;
  }
};

const foo = new Foo();

That I would argue is more JavaScripty... But beauty is in the eye of beholder.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Nov 14, 2017

That method of creating a "class" (with a closure) is not how ES6 classes work at all. It's an entirely different sort of object, one with no sugared equivalent in ES6 or TS

@unional
Copy link
Contributor

unional commented Nov 14, 2017

That method of creating a "class" (with a closure) is not how ES6 classes work at all

Yeah, sadly. :)

@renatomariscal
Copy link

renatomariscal commented Sep 20, 2018

I have the same issue: want to mock a type, and I am annoyed by having to include non-public members.
To overcome that, I am using a pattern of reducing with Pick:

class Identity {
    private id: string = "secret agent";
    public sameAs(other: Identity) {
        return this.id.toLowerCase() === other.id.toLowerCase();
    }
}

class MockIdentity implements Pick<Identity, 'sameAs'> {
    public sameAs(other: Identity) { return false; }
}

Would be convenient to have in the language an easy way to "Pick all public member".

@jandsu
Copy link
Author

jandsu commented Sep 20, 2018

+1 for the "Pick all public member" feature request, something like:

Public<Identity>

@tao-cumplido
Copy link
Contributor

@renatomariscal @jandsu
You can create such a type easily yourself with keyof, it only picks the public members.

type Public<T> = { [P in keyof T]: T[P] }

@fennecbutt
Copy link

fennecbutt commented Apr 26, 2019

Allowing the private fields to be missing would be an enormous problem, not some trivial soundness issue.

Consider this code:

class Identity {
  private id: string = "secret agent";
  public sameAs(other: Identity) {
    return this.id.toLowerCase() === other.id.toLowerCase();
  }
}

class MockIdentity implements Identity {
  public sameAs(other: Identity) { return false; }
}

MockIdentity is a public-compatible version of Identity but attempting to use it as one will crash in sameAs when a non-mocked copy interacts with a mocked copy.

I also think it'd be nice to have it behave in the expected way: public interface checked only.
I understand that the current implementation was chosen because of limitations with JS, but it's unintuitive and makes unit testing especially horrible; nobody wants to have to reimplement every single class in their entireity for their mocks.

From what I can see, as long as TS screams about attempted external private/protected property access then there's no issue, because this is already disallowed anyway?

Following up, I'm pretty happy using new MockFox() as Fox for unit testing, it's protected by sufficient overlapping being required, at least.

@trusktr
Copy link
Contributor

trusktr commented Dec 5, 2019

I believe that the type system should require one not to implement any private field from a ParentClass used in implements ParentClass (because that doesn't make any sense), but still require implementing protected and public properties.

But if property characteristics (accessibility levels of properties, etc) could live inside of types and interfaces, then we could selectively choose those parts with special mapped types. For example, It could look like implements PublicMembers<ParentClass> where PublicMembers is a mapped type that returns only the public members of the type passed into its generic parameter.

Perhaps a required important limitation of such a feature would be that the accessibility characteristics of properties would only be useful inside of extends and implements expressions.

I could really benefit from type/interfaces containing property characteristics (like class types do) in #35416.

AllanZhengYP added a commit to AllanZhengYP/aws-sdk-js-v3 that referenced this issue Jan 15, 2020
Sometimes yarn/npm will install different version of protocol-http
package under individual clients and root node_modules because of
hoisting policy. Private class member will block the ts compiling

Reference: microsoft/TypeScript#18499
AllanZhengYP added a commit to aws/aws-sdk-js-v3 that referenced this issue Jan 15, 2020
Sometimes yarn/npm will install different version of protocol-http
package under individual clients and root node_modules because of
hoisting policy. Private class member will block the ts compiling

Reference: microsoft/TypeScript#18499
@spion
Copy link

spion commented Mar 20, 2020

This effectively creates strict-nominal types.

It's also still causing problems across many projects which have to exactly align library versions to the smallest number in order to make everything build. For example, see microsoft/tsdoc#223

Yarn's strategy is to install the newest possible dependency for each requester, even when installing a slightly older dependency will result with fewer duplicates. This is a sound strategy because the latest version is likely to include patches and security fixes.

Solution 1: Get rid of this check

For those that want stricter "nominal-like" types, they can use a brand instead:

class Identity {
    ' brand' = 'mypackage.mybrand.v1' as const
}


class Identity2 {
    ' brand' = 'mypackage.mybrand.v2' as const
}

declare let val1: Identity;

let val: Identity2 = val1;

resulting with the error:

Type 'Identity' is not assignable to type 'Identity2'.
  Types of property '' brand'' are incompatible.
    Type '"mypackage.mybrand.v1"' is not assignable to type '"mypackage.mybrand.v2"'.(2322)

Solution 2: Remove the private fields when generating declaration files

There is no need for declaration file consumers to know there are private fields.

Cons:

  • Extending exported classes might result with an accidental override of private fields

Future solution:

  • Use the new "truly private" fields.

Solution 3: Ask library authors not to pin dependencies to exact version

TBD: research why library authors do this and determine viability.

I am in favor of solution 1. TypeScript can still add the private fields in .d.ts files but only check for clashes during inheritance, not assignment. It also makes TypeScript fully align with the structural / branded types strategy which seems to be the most viable option given the current state of the ecosystem

AllanZhengYP added a commit to AllanZhengYP/aws-sdk-js-v3 that referenced this issue Mar 20, 2020
Sometimes yarn/npm will install different version of protocol-http
package under individual clients and root node_modules because of
hoisting policy. Private class member will block the ts compiling

Reference: microsoft/TypeScript#18499
trivikr pushed a commit to trivikr/aws-sdk-js-v3 that referenced this issue Mar 20, 2020
Sometimes yarn/npm will install different version of protocol-http
package under individual clients and root node_modules because of
hoisting policy. Private class member will block the ts compiling

Reference: microsoft/TypeScript#18499
trivikr pushed a commit to trivikr/aws-sdk-js-v3 that referenced this issue Mar 24, 2020
Sometimes yarn/npm will install different version of protocol-http
package under individual clients and root node_modules because of
hoisting policy. Private class member will block the ts compiling

Reference: microsoft/TypeScript#18499
trivikr pushed a commit to trivikr/aws-sdk-js-v3 that referenced this issue Mar 24, 2020
Sometimes yarn/npm will install different version of protocol-http
package under individual clients and root node_modules because of
hoisting policy. Private class member will block the ts compiling

Reference: microsoft/TypeScript#18499
srchase pushed a commit to srchase/smithy-typescript that referenced this issue Mar 21, 2023
…ang#737)

Sometimes yarn/npm will install different version of protocol-http
package under individual clients and root node_modules because of
hoisting policy. Private class member will block the ts compiling

Reference: microsoft/TypeScript#18499
srchase pushed a commit to smithy-lang/smithy-typescript that referenced this issue Mar 23, 2023
Sometimes yarn/npm will install different version of protocol-http
package under individual clients and root node_modules because of
hoisting policy. Private class member will block the ts compiling

Reference: microsoft/TypeScript#18499
srchase pushed a commit to smithy-lang/smithy-typescript that referenced this issue Apr 17, 2023
Sometimes yarn/npm will install different version of protocol-http
package under individual clients and root node_modules because of
hoisting policy. Private class member will block the ts compiling

Reference: microsoft/TypeScript#18499
srchase pushed a commit to srchase/smithy-typescript that referenced this issue Apr 19, 2023
…ang#737)

Sometimes yarn/npm will install different version of protocol-http
package under individual clients and root node_modules because of
hoisting policy. Private class member will block the ts compiling

Reference: microsoft/TypeScript#18499
srchase pushed a commit to smithy-lang/smithy-typescript that referenced this issue Apr 26, 2023
Sometimes yarn/npm will install different version of protocol-http
package under individual clients and root node_modules because of
hoisting policy. Private class member will block the ts compiling

Reference: microsoft/TypeScript#18499
@electrovir
Copy link

electrovir commented Mar 27, 2024

A problem I've run into here is that there's no way (that I've yet found) to extract private members from a class instance type (keyof only extracts public members) so if I'm using mapped types, there's no way for them to know ahead of time that they're missing the private members.

Also, since all members are public at run time, this means that the mapped types are completely wrong if the run-time code is iterating over all members.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs The issue relates to how you learn TypeScript
Projects
None yet
Development

No branches or pull requests