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

chore(sdk): refactor connections API #3814

Merged
merged 8 commits into from
Aug 16, 2023
Merged

chore(sdk): refactor connections API #3814

merged 8 commits into from
Aug 16, 2023

Conversation

Chriscbr
Copy link
Contributor

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

/** @internal */
public readonly _connections: Connection[] = [];

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

  • 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 requested a review from eladb August 14, 2023 19:54
@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 16, 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 16, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 16, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify bot added a commit that referenced this pull request Aug 16, 2023
mergify bot added a commit that referenced this pull request Aug 16, 2023
@mergify mergify bot merged commit 3a82aa6 into main Aug 16, 2023
@mergify mergify bot deleted the rybickic/remove-resource branch August 16, 2023 19:01
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.25.15.

@eladb
Copy link
Contributor

eladb commented Aug 22, 2023

I love this direction. See my comment on the Display PR. I think we should merge std.Display and std.Connections into std.Node. This is all node-level general-purpose metadata.

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.

4 participants