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

Wing Platform overrides do not work on extended classes #6093

Open
hasanaburayyan opened this issue Mar 29, 2024 · 5 comments · May be fixed by #6099
Open

Wing Platform overrides do not work on extended classes #6093

hasanaburayyan opened this issue Mar 29, 2024 · 5 comments · May be fixed by #6099
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler

Comments

@hasanaburayyan
Copy link
Collaborator

I tried this:

when extending in non-entrypoint files

// main.w

bring "./lib.w" as lib;

let b = new lib.CustomBucket();

// lib.w

bring cloud;

pub class CustomBucket extends cloud.Bucket {}

Produces preflight code like

class CustomBucket extends (this.node.root.typeForFqn("@winglang/sdk.cloud.Bucket") ?? cloud.Bucket) {

The issue is that within this non-entrypoint preflight code, the class this.node.root happens at the top level where there is no this.

We need to somehow provide the App or PlatformManager to non entrypoint files so they can call app.node.root

This happened:

resource instantiating does not go through proper polycon hook process.

I expected this:

No response

Is there a workaround?

No response

Anything else?

No response

Wing Version

No response

Node.js Version

No response

Platform(s)

No response

Community Notes

  • Please vote by adding a 👍 reaction to the issue to help us prioritize.
  • If you are interested to work on this issue, please leave a comment.
@hasanaburayyan hasanaburayyan added the 🐛 bug Something isn't working label Mar 29, 2024
@hasanaburayyan
Copy link
Collaborator Author

hasanaburayyan commented Mar 29, 2024

Opening this issue because Im going to fix half the problem in: #6095, which is defined in #4791 but there is a larger refactor that needs to happen

mergify bot pushed a commit that referenced this issue Mar 29, 2024
**NOTE: ** This is really actually only a bandaid solution. It fixes half the problem.

There is a larger issue at hand described in: #6093 

The real issue is that we need a way to pass App instance or Platform manager to non-entrypoint files to use methods like `typeForFqn` or `new` but theres a pretty nasty cyclical dependency issue. 

currently working on another pr, that will probably take more time, in the meantime this unblocks: #4791

## Checklist

- [ ] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [ ] Description explains motivation and solution
- [ ] Tests added (always)
- [ ] Docs updated (only required for features)
- [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
@staycoolcall911
Copy link
Contributor

I just wanted to point out what are the functional implications of this issue for users:

They can't both add a custom platform that overrides a specific resource factory and also extend that same resource in their project (but only in non-entrypoint files).

I'm struggling with finding an example of a real-world use case for doing both resource customizations in the same Wing project.

@Chriscbr
Copy link
Contributor

Chriscbr commented Apr 1, 2024

One use case might be "I'm using the AWSCDK Wing platform for base implementations of cloud resources, and I'm extending those classes in my own Wing app"

@Chriscbr Chriscbr linked a pull request Apr 2, 2024 that will close this issue
5 tasks
@Chriscbr Chriscbr changed the title Extending classes in non-entrypoint files does not go through polycon flow Wing Platform overrides do not work on extended classes Jun 3, 2024
@Chriscbr
Copy link
Contributor

Chriscbr commented Jun 3, 2024

I think this bug also applies to non-entrypoint files, see the example below:

// main.w
bring "@cdktf/provider-aws" as aws;

pub class MyBucket extends aws.s3Bucket.S3Bucket {}

new MyBucket();
// my-platform.js
const aws = require("@cdktf/provider-aws");

exports.Platform = class MyPlatform {
  target = "tf-aws"
  newInstance(type, scope, id, props) {
    if (type === "@cdktf/provider-aws.s3Bucket.S3Bucket") {
      return new aws.s3Bucket.S3Bucket(scope, id, {
        ...props,
        bucketPrefix: "foo",
      })
    }
  }
}

A possible solution could be to refactor the API of Wing Platforms so instead of directly constructing instances for you, a platform returns types instead:

exports.Platform = class MyPlatform {
  target = "tf-aws";
  typeForFqn(fqn) {
    if (fqn === "@cdktf/provider-aws.s3Bucket.S3Bucket") {
      return class extends aws.s3Bucket.S3Bucket {
        constructor(scope, id, props) {
          super(scope, id, {
            ...props,
            bucketPrefix: "bar",
          })
        }
      }
    }
  }
}

@hasanaburayyan
Copy link
Collaborator Author

@Chriscbr yea this is the same issue. I think for sure typeForFqn needs to be implemented by the platform. Though the core of the issue is that in preflight.js files for non-entrypoint files we still have this issue:

class CustomBucket extends (this.node.root.typeForFqn("@winglang/sdk.cloud.Bucket") ?? cloud.Bucket) {

Where this.node... has no this to reference. So we need some sort of global polycon factory (which has proven challenging with cyclical dependencies) OR I think we can use that nifty little trick of changing an object's prototype in its constructor (which I have yet to try implementing)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler
Projects
Status: 🤝 Backlog - handoff to owners
Development

Successfully merging a pull request may close this issue.

3 participants