Use of "private" properties prevents multiple versions of a module in the same closure

[eladb]

Due the fact that when a class has private/protected members, TypeScript uses nominal type checking instead of structural type checking, different versions of the same class would not be compatible.

See details and example in microsoft/TypeScript#28128.

Bottom line, it appears that using "private" members is not practical for class libraries since it means classes cannot be used across versions and it is a common use case for ecosystems to have multiple versions of the same class in an application's closure.

To ensure that this cannot happen with the CDK and AWS Construct Library, I am proposing that jsii will not allow the use of TypeScript's private modifier. Instead it will recommend that users prefix a member with an underscore _, which will be hidden from jsii languages and is a common practice in JavaScript to denote private members.

The next step to enable multiple major versions is to allow jsii-runtime to load multiple versions into a module's closure. At the moment it is prohibited (aws/jsii#61).

[rix0rrr]

I imagine it's just the typechecker that's affected, so shouldn't have any impact on jsii.

But this is a supposition :).

[eladb]

@rix0rrr you are right, but the current implementation of the jsii-runtime has a limitation (aws/jsii#61) that won't allow multiple versions of the same module to be loaded into the runtime, so in a sense those are related.

[eladb]

Related: microsoft/TypeScript#8346

[eladb]

Here's a repro:

repro.zip

[eladb]

Another discussion on the topic: microsoft/TypeScript#7755

Basically, private members do not participate in structural typing intentionally in order to protect from overriding private implementation.

As much as I can see the purist reasoning behind it (and it seems like the TypeScript team doesn't intend to change things), we need a solution that will allow multiple versions of @aws-cdk/cdk to work together. This is especially tricky around base classes like Construct and Resource.

To be pragmatic about this, I'd like to propose that for these base classes, we will not use private members, but rather just use _ prefix to indicate those members are private. This is not an uncommon convention in JavaScript and jsii will hide these member from native languages.

@rix0rrr @RomainMuller I wonder what you guys think about this.

[rix0rrr]

I HATE EVERYTHING ABOUT IT.

But seriously.

I'd like to see automation around this, and preferably I wouldn't have to defile my code to work around these kinds of issues. I would like to see automatic translation of private xxx() to public _xxx() at the compilation phase. Either by postprocessing on the generated .js and .d.ts files, or by writing a compiler plugin.

[eladb]

I agree. It’s a huge limitation of private members in typescript, which I am not 100% sure they realize. I’ll chime in on one of these conversations to present our use case and ask for guidance.

In the meantime, we could either block the usage of private members at the Jsii level (tell you to use underscore and explain why) or just solve this locally for cdk.Construct and other core base classes, so that we could start versioning modules separately.

[RomainMuller]

I share @rix0rrr's philosophical issue with this. It is uncanny to me that I cannot use privates as pure implementation details. However, Javascript is what it is, and from that perspective I guess we have to preserve usability of the CDK.

[eladb]

This is actually a Typescript thing. Whether you use the CDK or not, if you wanted to use TypeScript to vend a class library, you wouldn't want to use "private" properties. I know, it's weird.

[yandy-r]

I don't like it, but a limitation of the language is a limitation. Based on the issue MS/Typescript rep adding interfaces for all types seems like a major headache. I'm not sure how my OCD will handle the _ everywhere, but improving functionality and practicality right now to get real use cases to me is more important than a few semantics.

I do agree with @rix0rrr that maybe in the future some logic and automation around this would be useful, though for now, your proposal seems fair.

[christophercurrie]

What does embracing the recommendation of using interfaces look like? The CDK could still provide base classes, but arguments themselves would always take the interface. It's a huge pain for the library to rename all the arguments, but it appears the cleanest from both the end user and the cross-language perspective.

[eladb]

The implication is that we will have to define an interface for every exported class in every module and somehow enforce that only those interfaces are used to reference these classes... doesn’t seem realistic

[eladb]

@huijbers proposed that the only reason typescript actually exposes private properties in type declarations is to allow objects to reference private properties of other objects of the same type. This was also mentioned in one of the typescript discussions (microsoft/TypeScript#7755 (comment)), another reason was to ensure that derived classes do not override the private property by accident.

@huijbers proposed to modify the typescrip compiler and add a mode which won’t private propert declarations, and perhaps can even enforce accessing private properties that are not via “this”.

What do you guys think? Maybe we can propose this as a contribution to typescript?

[christophercurrie]

Can you provide a link to that proposal? I can see it being valuable for some use cases.

A related question would be, what are private members being used for, and is there a way to eliminate them altogether, perhaps by making more types immutable?

[eladb]

In a conversation with @rix0rrr, he suggested that perhaps the correct modeling for dependencies that are exposed outside of a module's API is to use peerDependencies instead of normal dependencies, and I tend to agree.

For jsii code (CDK, AWS Construct Library, etc), we could enforce this at the jsii level: if a module's public API includes types from external modules, jsii can require that those are defined as peerDependencies instead of normal dependencies.

The problem is that we cannot enforce this for modules written by 3rd parties. We can recommend, guide and also make sure our templates are using peerDependencies for things like @aws-cdk/cdk (which by definition is exposed in the API), but we can't really enforce it as far as I know.

[eladb]

Indeed peerDependencies seem like the correct mechanism, but we from this article it seems like we must specify any of these dependencies both as "peerDependencies" and normal "dependencies". This will ensure that if a peer is not installed, npm (starting from version 3) will install it for you. If you only specify dependencies as peers, it means npm will require that you install all peers manually, and that's not sustainable.