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

Dependency injection for user defined resources #4930

Open
eladb opened this issue Nov 14, 2023 · 10 comments
Open

Dependency injection for user defined resources #4930

eladb opened this issue Nov 14, 2023 · 10 comments
Labels
✨ enhancement New feature or request 📐 language-design Language architecture 📚 libraries Wing Libraries needs-discussion Further discussion is needed prior to impl 👠 platforms Issues relating to Wing Platform Providers 🎨 sdk SDK

Comments

@eladb
Copy link
Contributor

eladb commented Nov 14, 2023

Today, Wing has mechanism that allows us to define and inject concrete Wing SDK resources based on the target platform defined during compilation.

When authoring user defined classes today, users use a hacky pattern where they use a proxy type and then explicitly check for WING_TARGET in their code.

As an example, check out https://github.com/winglang/winglibs/blob/main/containers/workload.w

Now that we are starting to write more winglibs, such as postgres, the lack of an official way to express this in wing is starting to impact more and more users.

We need a better way.

@staycoolcall911 staycoolcall911 added ✨ enhancement New feature or request 🎨 sdk SDK needs-discussion Further discussion is needed prior to impl labels Nov 14, 2023
@Chriscbr
Copy link
Contributor

I can offer an idea for a solution, though I haven't thought through all of the edge cases. To recap, suppose I have a class implemented once for each platform:

class BucketAws {
  // fun amazon logic
}

class BucketAzure {
  // fun microsoft logic
}

Writing these classes works well. If I want the classes to share some logic, I could have BucketAws and BucketAzure both extend a base class. The initial pain point today is the boilerplate you have to write to unify them:

class Bucket {
  new(props) {
    if util.env("WING_TARGET") == "tf-aws" {
      this.inner = new BucketAws(props);
    } else if util.env("WING_TARGET") == "tf-azure" {
      this.inner = new BucketAzure(props);
    } else {
      throw "unsupported target";
    }
  }
  
  inflight put(key: str, value: str): void {
    return this.inner.put(key, value);
  }
}

I think an ergonomic solution to this would be if I could just specify on each class which platform it's implemented for:

class Bucket for "tf-aws" {
  // fun amazon logic
}

class Bucket for "tf-azure" {
  // fun microsoft logic
}

Boom. Whenever new Bucket is called, the compiler automatically instantiates the right one for you. We have some technology for this in place already via Wing platforms.

But there's a catch -- how do we ensure the Bucket class has the same APIs on all targets? For example, what if you forgot to implement put on the Azure Bucket class? That ought to get caught at compile time.

One idea is if there's multiple classes of the same name with different "for" clauses, the compiler will eagerly type check both of them and then validate that they all have the same public API signatures (including their constructors). If there's a discrepancy, you'd get an error in all corresponding locations:

class Bucket for "tf-aws" {
  // error: "tf-aws", "tf-azure" implementations of class "Bucket" have different signatures for "put"
  inflight put(key: str): void {}
}

class Bucket for "tf-azure" {
  // error: "tf-aws", "tf-azure" implementations of class "Bucket" have different signatures for "put"
  inflight put(key: str, value: str): void {}
}

Since you haven't specified a common interface for Bucket up front, the compiler would be pretty strict in this scenario. If there's a public method named put() on one class, then put() must be implemented on all targets - no exceptions. If a public method returns Dog on one class, and Cat on the other class, the compiler doesn't try to find a common superclass return type between them.

But let's say you wanted more flexibility. Then Wing could let you define an abstract class up front:

abstract class Bucket {
  new(...BucketProps);
  inflight put(key: str: value: str): void;
}

Now, the compiler only needs to check that class Bucket for "tf-aws" satisfies abstract class Bucket, and class Bucket for "tf-azure" satisfies abstract class Bucket.

@MarkMcCulloh
Copy link
Contributor

I like the abstract and for ideas for this. I don't think it should rely on classes sharing a name, though, I think both users and the compiler will appreciate a more explicit link. The basis of this could always be an abstract class to serve as the contract and a way to share some implementation details:

pub abstract class Bucket {
  pub someField: number
  new(props: ...BucketProps) {
    this.someField = 100;
  }

  protected sharedFunctionality() {
    // Will probably be common to actually implement functions in abstract classes
  }

  inflight abstract put(key: str: value: str): void;
}

Then model the linkage with normal inheritance (with all the benefits and drawbacks), with the added twist of for

class Bucket extends cloud.Bucket for "tf-aws" {
  // inherited constructor and members
  // errors if no abstract members are implemented

  inflight put(key: str: value: str): void {
    // ...
  }
}

This implies of course that you'd be allowed to do new Bucket(). Normally constructing abstract classes would be crazy, but considering it's basically one of the main features of wing right now I think it would make sense.

@staycoolcall911 staycoolcall911 added the 📚 libraries Wing Libraries label Nov 15, 2023
@eladb
Copy link
Contributor Author

eladb commented Nov 15, 2023

i like the class AwsBucket extends cloud.Bucket for "tf-aws" direction.

  • what happens if there are two implementation of cloud.Bucket for "tf-aws"?
  • how can one install and use a library that just implements Y for "z" and override the default behavior?
  • can i extend/compose AwsBucket and customize some of it's functionality?
  • is it possible to extend AwsBucket and change it's target somehow or is the target embedded in the type and cannot be changed?

@ekeren
Copy link
Collaborator

ekeren commented Dec 12, 2023

We are assuming that cloud.Bucket is a class, while (at least for me) it makes more sense that cloud.Bucket would be an interface.

In this case, Wing novelty is the fact that you can call new on an interface, and it is the responsibility of the platform to provide the implementations of interfaces

@Chriscbr
Copy link
Contributor

Notes from a recent design discussion:

  • Before we commit to for "target" syntaxes or other sugar, it's worth first figuring out the data model for the factories in each platform where the dependency injection is performed (some kind of mapping?), and consider whether to add a configuration file or some other declarative syntax for augmenting existing factories or creating new factories.
  • Defining the API for a dependency injectable class through an abstract class and through an interface are both appealing options.
    • The issue with interfaces is that in most OO languages, an interface does not enforce the constructor signature.
    • The issue with abstract classes is that in most OO languages, a child class is allowed to have a different constructor signature than its base class.
    • We could opt to use interfaces and decide that Wing interfaces are allowed to specify a signature for new(). In these cases, any class implementing the interface must have a matching constructor signature.
    • We could opt to use abstract classes, but it would require disallowing OO patterns where a subclass extending an abstract class changes its constructor.

Copy link

Hi,

This issue hasn't seen activity in 60 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days.
Feel free to re-open this issue when there's an update or relevant information to be added.
Thanks!

@eladb
Copy link
Contributor Author

eladb commented May 15, 2024

@Chriscbr @hasanaburayyan what's the status of this? Any chance we can get this prioritized... Feels like we are accumulating lots a lot of debt in winglibs resulting from this missing capability.

@hasanaburayyan
Copy link
Contributor

If we want to take more time to plan out the syntax sugar for this, a potential interim solution could be to support writing Wing platforms in Wing, then could instead have implicit platforms that just implement newInstance

(Not that Im advocating for pushing this off just thinking outloud)


But Im all for prioritizing this and just moving forward with the for and abstract classes

@eladb
Copy link
Contributor Author

eladb commented May 16, 2024

I think the problem is that newInstance doesn't get called for wing-defined classes, only for JSII classes.

@Chriscbr Chriscbr added the 📐 language-design Language architecture label Jul 24, 2024
@Chriscbr
Copy link
Contributor

Just wanted to share an observation I had related to the challenge of supporting extending classes (after recently implementing #6490).

When someone extends a concrete class, it's normal that within most OO languages, inside the child class implementation, you're able to access the parent's implementation (fields, methods) as long as they're accessible (either public or protected).

But if you're extending an abstract (in Wing, this means dependency-injectable) class, you don't necessarily know the implementation of the class you're extending. So it's important that the Wing compiler doesn't let you do something weird like this:

class Bucket for "tf-aws" {
  pub field1: str;
  new() {
    this.field1 = // ...
  }
}

class Bucket for "tf-azure" {
  pub field2: num;
  new() {
    this.field2 = // ...
  }
}

// who are we extending? an abstract "Bucket" or one of the concrete "Bucket" classes above?
class SuperBucket extends Bucket {
  new() {
    super();
    this.field1 = "hey"; // error: field1 might not exist (???)
  }
}

I think the design suggested by Mark where you're required to define some abstract class probably makes the most sense for addressing this ambiguity. In that case, the above code would just error as soon as it's discovered that there are two Bucket classes with the same name. The correct code would look closer to:

pub abstract class Bucket {
  pub field1: number = 0;
}

class BucketTfAws extends cloud.Bucket for "tf-aws" {
  new() { this.field1 = 3; }
}

class BucketTfAzure extends cloud.Bucket for "tf-azure" {
  new() { this.field1 = 5; }
}

pub class SuperBucket extends Bucket {
  new() {
    super();
    this.field1 *= 2;
  }
}

@Chriscbr Chriscbr added the 👠 platforms Issues relating to Wing Platform Providers label Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request 📐 language-design Language architecture 📚 libraries Wing Libraries needs-discussion Further discussion is needed prior to impl 👠 platforms Issues relating to Wing Platform Providers 🎨 sdk SDK
Projects
Status: 🤝 Backlog - handoff to owners
Development

No branches or pull requests

6 participants