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

Cannot extend non-Wing constructs #3717

Closed
eladb opened this issue Aug 6, 2023 · 6 comments · Fixed by #3884
Closed

Cannot extend non-Wing constructs #3717

eladb opened this issue Aug 6, 2023 · 6 comments · Fixed by #3884
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler

Comments

@eladb
Copy link
Contributor

eladb commented Aug 6, 2023

I tried this:

bring "cdk8s" as cdk8s;

class MyChart extends cdk8s.Chart {
  init() {
    new cdk8s.ApiObject(kind: "Pod", apiVersion: "v1");
  }
}

let c = new MyChart();

This happened:

ERROR: this._addInflightOps is not a function

I expected this:

To work

Is there a workaround?

No response

Component

Compiler, SDK

Wing Version

No response

Node.js Version

No response

Platform(s)

No response

Anything else?

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.
@eladb eladb added the 🐛 bug Something isn't working label Aug 6, 2023
@monadabot monadabot added this to Wing Aug 6, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New - not properly defined in Wing Aug 6, 2023
eladb added a commit that referenced this issue Aug 6, 2023
When trying to bring `cdk8s` and extend `Chart`, the call to `this._addInflightOps()` fails because non-wing constructs don't have this method.

The fix is to use `?.()` (e.g. `this._addInflightOps?.()`) in order to chain the undefined.

Added a test which verifies that cdk8s can be brought and used.

Fixes #3717
@staycoolcall911 staycoolcall911 moved this from 🆕 New - not properly defined to 🏗 In progress in Wing Aug 8, 2023
@Chriscbr
Copy link
Contributor

Chriscbr commented Aug 14, 2023

@eladb I'm curious if using composition is a workaround for your use case? (trying to understand priority of this)

mergify bot pushed a commit that referenced this issue Aug 16, 2023
Background: in order for Wing classes to support extending non-Wing classes (#3717), I'm trying to remove the `Resource` class in the SDK so that all classes generated by our compiler just need to extend the ol' [`construct.Construct`](https://github.com/aws/constructs/tree/10.x) class -- the "base class" of the JSII ecosystem. This is a tricky effort since `Resource` is used in many places, and it collected a lot of responsibilities over the past year.

This PR takes a first step towards that by removing the connection-related information from the `Resource` class:

https://github.com/winglang/wing/blob/1c9954e50ebdd4f2b290e91665c920fb504c9cb7/libs/wingsdk/src/std/resource.ts#L222-L223

Instead, we now store the information inside the root of the construct tree. It can be accessed through `Connections.of(x)` where `x` is any class instance. See `connections.ts` for the full implementation (its short).

With this refactoring, I took the opportunity to clean up how connections are modeled. Previously, we were storing each connection twice inside `tree.json` (once next to the connection's source, and a second time next to the connection's target). Now, we just store each connection once, in a flat array inside `connections.json`. (I wasn't sure if it makes more sense to put it in a separate file - I'm interested in feedback on this). I also removed the `implicit` field which wasn't used by the SDK or console anywhere.

## Checklist

- [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [x] Description explains motivation and solution
- [x] 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)*.
@Chriscbr Chriscbr mentioned this issue Aug 18, 2023
5 tasks
@mergify mergify bot closed this as completed in #3884 Aug 24, 2023
mergify bot pushed a commit that referenced this issue Aug 24, 2023
As part of the effort to fix #3717, I'm trying to refactor some parts of the SDK so that we do not need to rely on the `std.Resource` class.  The issue this PR addresses is that the `Resource` class has an extra property named `display` which is not available if you extend `cdk8s.Chart`:

```js
bring cdk8s;

class Bar {
  init() {
    this.display.title = "hi"; // OK
  }
}

class MyChart extends cdk8s.Chart {
  init() {
    this.display.title = "hello"; // error: Unknown symbol "display"
  }
}
```

The solution implemented by this PR is to standardize display property access through `std.Node`:

```js
bring cdk8s;

class Bar {
  init() {
    std.Node.of(this).title = "hi"; // OK
  }
}

// once #3717 is fixed
class MyChart extends cdk8s.Chart {
  init() {
    std.Node.of(this).title = "hello"; // OK
  }
}
```

`std.Node.of(this)` also allows you toaccess all other fields and methods normally available through the `constructs.Node` class.

Other changes:
- I've replaced the `_addInflightOps` method from `std.Resource` with an API contract where a class can (optionally) implement a method named `_getInflightOps()`. This removes one more of the places where the compiler assumes behavior that only exists on `std.Resource` and not on `constructs.Construct`. 
- I've removed the `Code` class from the SDK. I looked through our codebase and 100% of the usages have just been to access its `text` field, so the class isn't really doing anything important. Just using plain `string`'s seems like a nice simplification.

BREAKING CHANGE: `.display` property is no longer available in Wing classes. To change how classes are displayed in the Wing Console, use `std.Node.of(this)` and modify fields like `.title` and `.description`.

## Checklist

- [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [x] 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)*.
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Wing Aug 24, 2023
@Chriscbr Chriscbr reopened this Aug 24, 2023
@github-project-automation github-project-automation bot moved this from ✅ Done to 🤝 Backlog - handoff to owners in Wing Aug 24, 2023
@winglang winglang deleted a comment from monadabot Aug 25, 2023
@github-actions
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!

@github-actions github-actions bot added the Stale label Oct 25, 2023
@staycoolcall911
Copy link
Contributor

I believe this now works.
CC: @eladb, @Chriscbr

@github-project-automation github-project-automation bot moved this from 🤝 Backlog - handoff to owners to ✅ Done in Wing Oct 25, 2023
@Chriscbr
Copy link
Contributor

Thanks for the heads up @staycoolcall911. It looks like the immediate bug above was fixed (I think via #4640), but if I extend the example a little further with some inflight code, we still get errors:

bring "cdk8s" as cdk8s;
bring cloud;

let global_b = new cloud.Bucket();

class MyChart extends cdk8s.Chart {
  local_b: cloud.Bucket;
  init() {
    new cdk8s.ApiObject(kind: "Pod", apiVersion: "v1");
    this.local_b = new cloud.Bucket();
  }

  pub inflight save() {
    global_b.put("data.txt", "Hello World!");
    this.local_b.put("data.txt", "Hello World!");
  }
}

let c = new MyChart();
new cloud.Function(inflight () => {
  c.save();
});

error:

ERROR: unable to serialize immutable data object of type MyChart

target/main.wsim.988477.tmp/.wing/preflight.js:79
         _registerOnLift(host, ops) {
           if (ops.includes("handle")) {
>>           $Closure1._registerOnLiftObject(c, host, ["save"]);
           }
           super._registerOnLift(host, ops);

I think the root cause is our lifting system is still assuming std.Resource is in the class's hierarchy. Refactoring the SDK to remove std.Resource feels like the cleanest solution IMO

@Chriscbr Chriscbr reopened this Oct 25, 2023
@github-project-automation github-project-automation bot moved this from ✅ Done to 🤝 Backlog - handoff to owners in Wing Oct 25, 2023
@github-actions github-actions bot removed the Stale label Oct 26, 2023
Copy link

Hi,

This issue hasn't seen activity in 90 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!

@Chriscbr
Copy link
Contributor

Chriscbr commented Jun 3, 2024

From my testing it looks like the previous example now works. I was able to uncover a separate issue but it's more minor: #6629

Closing.

@Chriscbr Chriscbr closed this as completed Jun 3, 2024
@github-project-automation github-project-automation bot moved this from 🤝 Backlog - handoff to owners to ✅ Done in Wing Jun 3, 2024
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
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants