feat(sdk): inflights are not required to be resources #4993
Conversation
|
Console preview environment is available at https://wing-console-pr-4993.fly.dev 🚀 Last Updated (UTC) 2023-11-25 15:34 |
BenchmarksComparison to Baseline ⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜
⬜ Within 1.5 standard deviations Benchmarks may vary outside of normal expectations, especially when running in GitHub Actions CI. Results
Last Updated (UTC) 2023-11-25 15:42 |
There was a problem hiding this comment.
Looks like a worthy direction to explore. May the gods of our unit test and type system guide you safely to a better place.
(Gut feeling is that the non determinism of inflight identifiers could bite us in the ass, but I am willing and p give this a go).
Also, please don't forget the "wing first" tenet. Eventually our typescript api needs to be produced based on wing declarations.
|
@MarkMcCulloh , is this suppose to resolve #2853 |
|
@ekeren Not yet, although it's a step in that direction. This changes how the SDK works but the compiler will still be creating resources. This will need to change, but it is useful to change it here first (and it's backwards compatible) |
monadabot
added
the
⚠️ pr/review-mutation
MarkMcCulloh
dismissed
eladb’s stale review
PR has underdone significant change
|
Thanks for contributing, @MarkMcCulloh! This PR will now be added to the merge queue, or immediately merged if |
…ct app (#5055) Followup to #5054 (Accidentally changed sim instead of tf-aws) Also follow up to #4993 (see topic.ts, reverted a change to call of `addPermissionToInvoke`) Ran failing AWS tests locally this time to verify fix *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)*.
|
Congrats! 🚀 This was released in Wing 0.50.3. |
Huh?
The primary goal of this PR is to reduce the input required to create an inflight function in TS (See #4842) without necessarily overhauling the compiler (yet). Ideally, the minimum information required for an inflight is simply the code itself. However, because inflights are currently modeled as resources, they require a scope and id.
So the first change was to make a new non-resource interface, IInflight, encompassing the inflight contract. The most important part of this contract is that inflights must be liftable, a behavior currently unique to resources and certain other primitives. So I extracted the lift-related functions from IResource and slapped them on the new ILiftable (which both IInflight and IResource now extend).
But that created a new problem: lifting itself also currently requires a scope. The only usage of the scope was to be able to resolve tokens. This did not seem like a good enough reason to require scope, so I changed token resolution to be more of a global concern rather than a tree-level concern. This is dangerous, but it's mostly only dangerous when you try to deal with tokens in a multi-app scenario, which would be dangerous with our current approach anyways. So this is something we'll have to add some extra handling for eventually anyways.
Results
The primary outcome of this can be seen in the SDK unit tests, where the
Testing.makeHandler()now only requires the code and (optional) bindings. This is basically 1 or 2 steps away from an ideal TS experience.But wait nothing changed in winglang
The original purpose of representing inflights as resources was to ease the implementation of lifting in the compiler and generally unify the logic of inflights between inflight closures and inflight methods of preflight classes. This hasn't changed in this PR. Luckily, the class instance emitted by the wing compiler for inflights still satisfies IInflight. It just has some extra hidden resource stuff that is simply unused. Assuming this PR is wanted, I will do a followup to change the compiler as well. This will be a more complicated change and I think it's useful to basically get the backend working first.
Changes
Testing.makeHandlernow takes only code text and bindings. 9 billion tests were updated for this contract.convertBetweenHandlerschanged similarlyaddrnow do so with_hashinsteadthis.node.idused in resource ids when it's not necessary. The only time this should be necessary is if the resources is being created in the scope of this.node.scope insteadCounterconcept to help with the many places that we want to add a subresource many times and a simple incrementing number will suffice for uniqueness and clarityaddrwas often relied upon to make this unique, but that will no longer be viable. I think it's better this way anywaysBy submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.