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

feat(sdk)!: std.Node #3884

Merged
merged 16 commits into from
Aug 24, 2023
Merged

feat(sdk)!: std.Node #3884

merged 16 commits into from
Aug 24, 2023

Conversation

Chriscbr
Copy link
Contributor

@Chriscbr Chriscbr commented Aug 18, 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:

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:

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

  • Title matches Winglang's style guide
  • 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.

@Chriscbr Chriscbr marked this pull request as ready for review August 18, 2023 17:36
@Chriscbr Chriscbr requested a review from a team as a code owner August 18, 2023 17:36
@Chriscbr Chriscbr requested a review from MarkMcCulloh August 18, 2023 17:36
@monadabot monadabot added the ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! label Aug 18, 2023
@Chriscbr Chriscbr removed the ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! label Aug 18, 2023
libs/wingsdk/src/std/display.ts Outdated Show resolved Hide resolved
@Chriscbr Chriscbr changed the title feat(sdk)!: refactor Display API feat(sdk)!: std.Node Aug 22, 2023
@Chriscbr Chriscbr requested a review from eladb August 22, 2023 23:11
@Chriscbr
Copy link
Contributor Author

@eladb Updated the API. Leaving nodeof for a separate PR since it requires some refactoring (#2027)

Signed-off-by: monada-bot[bot] <[email protected]>
@monadabot monadabot added the ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! label Aug 22, 2023
@Chriscbr Chriscbr removed the ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! label Aug 23, 2023
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if we need to add a doc topic about nodes?

Maybe even just an initial thing with a reference to the construct documentation?

@mergify
Copy link
Contributor

mergify bot commented Aug 24, 2023

Thanks for contributing, @Chriscbr! This PR will now be added to the merge queue, or immediately merged if rybickic/refactor-display is up-to-date with main and the queue is empty.

@monadabot monadabot added the ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! label Aug 24, 2023
@Chriscbr Chriscbr removed the ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! label Aug 24, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 24, 2023

Thanks for contributing, @Chriscbr! This PR will now be added to the merge queue, or immediately merged if rybickic/refactor-display is up-to-date with main and the queue is empty.

@mergify mergify bot merged commit 8efecc2 into main Aug 24, 2023
@mergify mergify bot deleted the rybickic/refactor-display branch August 24, 2023 22:29
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.26.0.

mergify bot pushed a commit that referenced this pull request Aug 25, 2023
Fixes a regression from #3884 - tree.json was no longer being emitted with `sourceModule` information, causing some issues in the Wing Console. Thanks @polamoros for catching this 🙏 

## 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)*.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot extend non-Wing constructs
3 participants