Nominal type checking for private properties cannot be used in class libraries (TS2322)

[eladb]

I'd like to add our two cents to the discussions in #8346 and #7755.

We understand the desire to ensure that code can trust that private properties have consistent behavior across versions, but practically this means that it won't be possible to use private members class libraries. This is because it is common that multiple versions of the same class library will exist in the same dependency closure. If a class has private members, than suddenly two versions of the same class are not interchangeable, even if they are fully compatible (private + public).

Specifically to our use case: we are building the AWS Cloud Development Kit. Almost all CDK classes inherit from a base class Construct which is defined in one of the core libraries. This base class contains private properties.

Constructs form a tree, which is structured by supplying the parent construct upon initialization:

Sketch of the base class:

class Construct {
  private _children: Construct[];
  constructor(parent: Construct);
}

Now, both users of the CDK and vendors of 3rd party CDK libraries create constructs by extending the Construct base class.

Here's a common use case:

import { ThirdPartyConstruct } from '3plibrary';

class MyApp extends Construct {
  constructor(parent: Construct) {
    super(parent);

    new ThirdPartyConstruct(this); // add this guy as a child

    // my logic
  }
}

Now, say this user depends on CDK 1.0.0 and 3plibrary depends on the CDK 1.0.1, the base classes are not compatible anymore and the user will get the infamous Type 'MyApp' is not assignable to type 'Construct'. Types have separate declarations of a private property '_children'..

This is a major limitation of course for such a use case. At the moment we are considering to basically convert all private properties to public methods with underscore prefixes, but naturally that's not what we prefer.

Related: aws/aws-cdk#979

[eladb]

Apologies for the accidental submission before finishing the issue 👎

[weswigham]

The typical suggestion is to take an interface rather than the actual underlying class at your input positions; that way at your input positions where you've determined version mismatches shouldn't be an issue, you only need something conforming to the public API of your components, rather than a specific instance of them.

[eladb]

So, effectively the parent would be something like IConstruct instead of Construct. Correct?

[weswigham]

Yep:

interface IConstruct {
  // no members?
}
class Construct implements IConstruct {
  private _children: Construct[];
  constructor(parent: IConstruct);
}

class MyApp extends Construct {
  constructor(parent: IConstruct) {
    super(parent);

    new ThirdPartyConstruct(this); // add this guy as a child

    // my logic
  }
}

[eladb]

Is the proposal for all type references to only use interfaces? Sounds like a huge overhead for a large class library.

Here's another example:

1. lib-foo exports a type Foo (with private properties). It is published in two versions [email protected] and [email protected] (say both are structurally compatible).
2. [email protected] exports a function giveMeFoo(foo: Foo). It depends on [email protected]

Now, my app takes a dependency on [email protected] and [email protected].

This code will fail:

import { Foo } from 'lib-foo';
import { giveMeFoo } from 'lib-goo';

giveMeFoo(new Foo()); // TS2322

This practically means that we lost npm's ability to have multiple versions of the same library co-exist in the same closure.

Do I understand this correctly, or am I missing something? Seems like the guidance would either be to declare an interface for each class or to not use private properties altogether.

[eladb]

For posterity: what we eventually decided to do is to declare any dependency that includes types that are part of a module’s public API also as peerDependencies.

We realized this is the right way to ensure that types are semantically compatible, which is superior to structurely-compatible.