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

Hide Construct.dependencyRoots #2348

Closed
eladb opened this issue Apr 22, 2019 · 4 comments · Fixed by #2668 or MechanicalRock/tech-radar#14 · May be fixed by MechanicalRock/cdk-constructs#5, MechanicalRock/cdk-constructs#6 or MechanicalRock/cdk-constructs#7
Assignees
Labels
@aws-cdk/core Related to core CDK functionality package/awscl Cross-cutting issues related to the AWS Construct Library

Comments

@eladb
Copy link
Contributor

eladb commented Apr 22, 2019

We wanted to ensure that the base Construct public API is clean (besides node) and leaves room for the API of the actual construct. Somehow dependencyRoots snuck in.

Is there a way to get rid of this? Why even use an override here? Can't we do something like:

construct.node.addDependencyRoot(x);
@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 23, 2019

It's not about Construct.dependencyRoots, it's about IDependable.dependencyRoots, and that interface is also implemented by non-Constructs.

I have a feeling this crops up a bunch, where we want to implement some polymorphic facility for some classes without necessarily dispatching through the object instance (because then the facility needs to show in the public interface, like here).

We can take the implementation out of the class. Something like traits:

// Marker interface purely so customers know which object to pass where (makes this
// distinctly less trait-like, but more usable). Otherwise, we type the function `foo`
// below as `foo(target: any)`.
interface IDependable {
}

// Declaration of trait
abstract class DependableTrait {
  public abstract dependencyRoots;
}

// Declaration of class unsullied by trait signature
// Does have a marker interface to help type checking.
class MyDependableThing implements IDependable {
  // ...
  // nothing about dependencyRoots here
  // ...
}

// Trait implemented separately from subject class
class SomeTraitImplementation extends DepenableTrait {
  constructor(subject: MyDependableThing) { }
  public dependencyRoots() {
    // Implementation using this.subject here
  }
}

DependableTrait.implement(MyDependableThing, SomeTraitImplementation);

// Usage site
function foo(target: IDependable) {
  const roots = DependableTrait.get(target).dependencyRoots;
}

We could do the same for bind() and other implementation-concern API surface.

@RomainMuller RomainMuller changed the title Hide Construct.dependenyRoots Hide Construct.dependencyRoots Apr 23, 2019
@RomainMuller RomainMuller added the @aws-cdk/core Related to core CDK functionality label Apr 23, 2019
@eladb
Copy link
Contributor Author

eladb commented Apr 30, 2019

I like the marker interface but I am wondering if perhaps the traits can be simply implemented inline. Doesn't seem like a separate class buys us too much, or maybe I am missing something.

interface IDependable { }
interface IDependableTrait {
  dependencyRoots: ...
}

class MyDependableThing implements IDependable {
  constructor() {
    Trait.implement<IDependableTrait>(this, {
        dependencyRoots: ...
    });
  }
}

// usage
Trait.get<IDependableTrait>(target).dependencyRoots...

Having said that, I am wondering if this pattern is actually that prevalent. I don't know if "bind" lends itself nicely to this, and I think that as we move to "integration classes", we'll see much less of this need.

Can we figure something out for dependencyRoots for now and see how it works?

@eladb eladb added package/awscl Cross-cutting issues related to the AWS Construct Library and removed package/awscl Cross-cutting issues related to the AWS Construct Library labels May 1, 2019
@eladb eladb added the may-15 label May 14, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented May 15, 2019

wondering if perhaps the traits can be simply implemented inline. Doesn't seem like a separate class buys us too much

In your code, we're mapping instances to instances. In my code, I was mapping classes to classes.

The latter will be more efficient if we're going to have a lot of instances of the same class.


    Trait.implement<IDependableTrait>(this, {

I was thinking this should still be accessible over jsii, so no generics.


Having said that, I am wondering if this pattern is actually that prevalent

It also occurs in the ELB library, where LBs and listeners and groups have a bind()-like protocol between them, AND the objects themselves expose a public API that customers are expected to call.

@rix0rrr rix0rrr self-assigned this May 29, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented May 29, 2019

Here's another rub: because of structural typing, marker interfaces don't mean anything in TypeScript. That is, I can do the following:

interface IDependable {
}

function doSomething(dep: IDependable) {
}


doSomething(new SomethingCompletelyUnrelated());

And TS won't complain. We can still do this, but the type marker will only be honored in non-TS languages.

rix0rrr added a commit that referenced this issue May 29, 2019
`IDependable` is now a marker interface, and signals that there is
a `DependableTrait` implementation which can be retrieved which contains
the actual implementation of `dependencyRoots` for this instance.

Fixes #2348.
rix0rrr added a commit that referenced this issue May 29, 2019
`IDependable` is now a marker interface, and signals that there is
a `DependableTrait` implementation which can be retrieved which contains
the actual implementation of `dependencyRoots` for this instance.

Fixes #2348.
rix0rrr added a commit that referenced this issue May 30, 2019
`IDependable` is now a marker interface, and signals that there is
a `DependableTrait` implementation which can be retrieved which contains
the actual implementation of `dependencyRoots` for this instance.

Fixes #2348.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment