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

[RFC] Typescript experience for Wing #4842

Closed
MarkMcCulloh opened this issue Nov 8, 2023 · 18 comments
Closed

[RFC] Typescript experience for Wing #4842

MarkMcCulloh opened this issue Nov 8, 2023 · 18 comments
Assignees
Labels
✨ enhancement New feature or request needs-discussion Further discussion is needed prior to impl 🎨 sdk SDK

Comments

@MarkMcCulloh
Copy link
Contributor

MarkMcCulloh commented Nov 8, 2023

Design Tenets

  • "Wing first" - We optimize for authoring libraries in winglang and consuming them from TypeScript. No need to optimize for a TypeScript authoring experience.
  • We will design the TypeScript API to be as close to the the winglang experience as possible, because we assume we designed the winglang to be the ideal experience.
  • Users will use the wing CLI to compile, test and run Wing applications written in TypeScript.
  • DX is based on the construct programming model used by CDKs (new T(scope, id)).
  • "TypeScript native" - Wing for TypeScript won't require require customizations or extensions to the TypeScript compiler. In the future we might consider adding "sugar" or "helpers" using such mechanisms, but the base experience shouldn't require this.
  • The Wing IDE extension is not needed to use Wing for TypeScript.

Feature Spec

The Wing toolchain now supports TS/JS entrypoints. Commands like wing compile, wing test, and wing it can take a Typescript or Javascript file. This allows you to get the benefit of the Wing ecosystem (beautiful abstractions, testing, simulator, Wing Console, and Wing Cloud) without committing to winglang (yet!)

(example contains a proposal API for discussion, very much subject to change)

// main.ts
import { cloud, std, expect, inflight } from "@winglang/sdk";

// Final App depends on platform used
const app = new cloud.App();

// Familiar construct-base DX
const bucket = new cloud.Bucket(app, "Bucket");
const api = new cloud.Api(app, "Api");
const message = "Hi";

// System for modeling `inflight` functions
// Will need to explore mechanisms to lift preflight objects
const func = new cloud.Function(app, "Function", inflight({ bucket, api, message }, async (ctx, event) => {
  // can access inflight methods via the ctx context
  await ctx.bucket.put("stuff", event);

  // can reference preflight fields
  console.log(ctx.api.url);

  // error: basic static analysis can catch references to preflight value without ctx
  console.log(api.url);

  // can reference static data
  console.log(ctx.message);
}))

// The explicit lifting ({ func, bucket }) may be avoidable in a future version
new std.Test(app, "can call function", inflight({ func, bucket }, async (ctx) => {
  await ctx.func.invoke();
  assert(await ctx.bucket.exists("stuff"));
})

app.synth();
# All valid commands
wing compile main.ts
wing it main.ts
wing test main.ts
wing test -t tf-aws main.ts

MVP

  • The above example works with the given commands (no wing pack)
  • The code may have strict conventions (e.g. requires exactly 1 cloud.App instance that is .synth()ed at the end)
  • No automatic permissions granting. By default it will either be all or nothing, possibly requiring an API to grant permissions
  • Simple analysis for inflight functions to prevent access to "bare" preflight classes
  • Typechecking itself will not prevent you from accessing inflight from preflight, but the error will still be caught at build time.

Thoughts for later

  • Automatic id-generation would be a huge boost and is probably feasible
  • May be interesting to explore a non-constructy DX, e.g.
const app = cloud.App();
// use function, move all arguments into object
const bucket = cloud.Bucket({ scope: app, id: "Bucket" });
// ---
const app = cloud.App();
scope(app, () => {
 // Automatic scope and id with closures
 const bucket = cloud.Bucket();
})

Use Cases

  • Provide the benefits of wing to users who aren't ready to move to winglang
  • Ease adoption of winglang

Implementation Notes

After some initial proof-of-concept experimentation, the goal for this is to make the wing compiler capable of producing a library that is consumable via TS. Then, the library that powers this experience can be built with winglang.

Possible Necessity: https://github.com/microsoft/TypeScript/wiki/Writing-a-Language-Service-Plugin

Component

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.
  • If this issue is labeled needs-discussion, it means the spec has not been finalized yet. Please reach out on the #dev channel in the Wing Slack.
@MarkMcCulloh MarkMcCulloh added ✨ enhancement New feature or request needs-discussion Further discussion is needed prior to impl labels Nov 8, 2023
@monadabot monadabot added this to Wing Nov 8, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New - not properly defined in Wing Nov 8, 2023
@eladb
Copy link
Contributor

eladb commented Nov 9, 2023

This looks awesome!

Two questions:

  1. Why do we need bucket.client? I believe we figured out a way to synthesize a joint type, no?

  2. I think wing pack is a must. Didn't we say we wanted to create a wing library that exposes a shim to the cloud library?

@MarkMcCulloh
Copy link
Contributor Author

MarkMcCulloh commented Nov 9, 2023

Why do we need `bucket.client'? I believe we figured out a way to synthesize a joint type, no?

Doing an intersection type is possible and I'm still open to it, but I'm concerned about the DX. I think exposing a bunch of members that are unusable may be strange/confusing.
Wrapping everything inflight-related in a member doesn't feel too bad. Doing that, the only confusing part is that preflight methods are exposed while inflight.

I'll be doing more experiments to see what works though.

I think wing pack is a must. Didn't we say we wanted to create a wing library that exposes a shim to the cloud library?

I meant using wing pack with a TS entrypoint. wing pack will indeed need to produce a library consumable from TS from a wing entrypoint.
It may not be too hard to implement wing pack blah.ts assuming everything else is done, but I wanted to find some way to limit the scope of this large task.

@staycoolcall911 staycoolcall911 moved this from 🆕 New - not properly defined to 🏗 In progress in Wing Nov 9, 2023
@staycoolcall911 staycoolcall911 added this to the Wing Cloud 1.0 milestone Nov 9, 2023
@eladb
Copy link
Contributor

eladb commented Nov 9, 2023

Doing an intersection type is possible and I'm still open to it, but I'm concerned about the DX. I think exposing a bunch of members that are unusable may be strange/confusing.

As long as the inflight methods are async and will throw if called in preflight (during synthesis), I think that's a totally reasonable DX.

And I really love the seamless DX it can get us.

But we can definitely try these two alternatives.

I meant using wing pack with a TS entrypoint

Oh, yes, of course. You are right.

@MarkMcCulloh
Copy link
Contributor Author

As long as the inflight methods are async and will throw if called in preflight (during synthesis), I think that's a totally reasonable DX.

inflight methods are async, but not inflight fields. Of course we can still figure out a way to throw if we have phase mismatches, but the completions will look very messy if you have preflight and inflight stuff next to each other. This is possibly resolvable with a TSServer plugin, which will be part of the experimentation.

@eladb
Copy link
Contributor

eladb commented Nov 9, 2023

inflight fields

I think we will get rid of inflight fields anyway.

TSServer plugin

Cool!

mergify bot pushed a commit that referenced this issue Nov 11, 2023
Part of the effort of #4842

Will allow you to directly do `new cloud.Bucket` in TS directly and still get the benefit of the App factory.

Changed it so that you can no longer instantiate abstract classes, since there is no need for it anymore.

*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)*.
@skyrpex
Copy link
Contributor

skyrpex commented Nov 21, 2023

When I was building a type system for the dual interfaces during the early days, I did come up with something like this (pseudo-code...). The inflight function accepts a map of resources, and pass the inflight type as an option to the handler:

type WingResource<Preflight, Inflight> = Preflight;

type GetInflight<T> = T extends WingResource<any, infer Inflight> ? Inflight : never;

// Can do some magic here
const inflight = <Captures extends Record<string, WingResource<any, any>>>(captures, handler) => {
  // ...
};

So:

interface BucketPreflight {
  onCreate(handler: async () => Promise<void>): void;
}

interface BucketInflight {
  async put(key: string, value: string): void;
}

type Bucket = WingResource<
  BucketPreflight},
  BucketInflight,
>;

And the resulting DX is:

const bucket = new cloud.Bucket();

const func = new cloud.Function(app, "Function", inflight({ bucket }, async ({ ctx, event }) => {
  // can access inflight methods via ctx
  await ctx.bucket.put("stuff", event);
}))

@ShaiBer
Copy link
Contributor

ShaiBer commented Nov 22, 2023

@MarkMcCulloh this is awesome! goes beyond what I thought was possible to achieve with a rather small effort.

@skyrpex I like your proposed DX. But I don't fully understand how it can be integrated with Mark's design and at what costs and benefits

@MarkMcCulloh
Copy link
Contributor Author

MarkMcCulloh commented Nov 22, 2023

Thanks @skyrpex for writing that out

const bucket = new cloud.Bucket();
const func = new cloud.Function(app, "Function", inflight({ bucket }, async ({ ctx, event }) => {
  await ctx.bucket.put("stuff", event);
}))

The largest benefit of an approach like this is that it's significantly easier to implement, and additionally is completely able to be implemented without any extra tooling. It also does not conflict with how TS works (i.e. the bucket identifier does not change what it is across boundaries).

This was an approach I originally considered but was hoping to figure out a more novel approach for the experience to "feel" more like you're writing wing. After some experimentation though, I'm less optimistic that we can do something super fancy here. The .client part of the original post was already a compromise on that front.

Some more ideas related to @skyrpex's idea, I'm curious if any of these resonate with others:

const bucket = new cloud.Bucket();
const func = new cloud.Function(app, "Function", inflight({ bucket }, async (ctx, event) => {
// ctx is always the first positional argument. This makes things more consistent across inflight contracts (they don't always have an object as a request)
// the downside of course is if you don't want to use ctx, it'll be terribly ugly to ignore it
  await ctx.bucket.put("stuff", event);
}))
const bucket = new cloud.Bucket();
const func = new cloud.Function(app, "Function", inflight.lift({ bucket }).from(async (ctx, event) => {
// use a builder pattern
  await ctx.bucket.put("stuff", event);
}))
const bucket = new cloud.Bucket();
const func = new cloud.Function(app, "Function", Inflight.with({ bucket }).from(async (ctx, event) => {
// more like a static factory
  await ctx.bucket.put("stuff", event);
}))

// Should be more reasonable to allow inflight to be defined in different ways
// This basically gives us `extern`
const func2 = new cloud.Function(app, "Function2", Inflight.with({ bucket }).fromFile("./blah.ts"))

// or just
const func3 = new cloud.Function(app, "Function3", Inflight.from(async () => {
  console.log("I hope builtins work")
}))

@eladb
Copy link
Contributor

eladb commented Nov 22, 2023

I'm less optimistic that we can do something super fancy here

@MarkMcCulloh Why?

In our prototype we were able to get to this:

import { cloud } from "wingsdk";

const app = new cloud.App();
const bucket = new cloud.Bucket(app, "Bucket");

const fn = new cloud.Function(app, "Function", { bucket }, async (event) => {
  await bucket.put("hello.txt", "world");
});

I thought this was very good DX achieved by implementing all the inflight methods as async stubs of the preflight class (throws an exception during preflight synthesis) and implementing all the preflight methods as stubs of the inflight class (throws an exception during inflight runtime).

We also said we will need to generate some API for explicit binding/grants.

Did you run into issues with this approach?

@ekeren
Copy link

ekeren commented Nov 23, 2023

I feels like the ctx.bucket approach solves both the inline version (have the code in the same file) and the external file option. Basically the ctx approach allows me to move the function arround

@eladb
Copy link
Contributor

eladb commented Nov 23, 2023

How will that look like side by side? (what are the typings of the handler function?)

My understanding is that a lot of the magic comes from the context inferred type which is based on the capture map.

@ekeren
Copy link

ekeren commented Nov 23, 2023

How will that look like side by side?

¯_(ツ)_/¯, I am the product guy....

Seriously thought, I haven't dug into the technical details on how this magic work and what are the limitations.

@MarkMcCulloh / @skyrpex ?

mergify bot pushed a commit that referenced this issue Nov 25, 2023
## 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.makeHandler` now takes only code text and bindings. 9 billion tests were updated for this contract. `convertBetweenHandlers` changed similarly
- TokenResolvers are now globally registered and not tied to specific apps
- wingc adds a _hash private field to inflight closure resource classes to match the new IInflight (just an md5 hash)
- Many of the resources that deduped functions based on `addr` now do so with `_hash` instead
- Removed many occurrences of `this.node.id` used 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 instead
- Added a `Counter` concept 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 clarity
  - This was needed because the inflight `addr` was often relied upon to make this unique, but that will no longer be viable. I think it's better this way anyways

*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
Copy link
Contributor

Chriscbr commented Dec 6, 2023

One question on my mind in this hypothetical TS experience is how errors about using methods in the wrong phases would be surfaced to the user. (For example, when the user calls a preflight method in inflight, or inflight method in preflight). If errors only appear once your code is executed (and not while you're typing code in your IDE) that could be a setback.

I was playing around with some TypeScript and discovered a few styles of implementations that could meet the requirement of surfacing errors at compile time.

In this first one, all of the methods of Bucket and Function are defined in a single class, and some special type rules are used to enforce that you can't call inflight methods in preflight. This is neat, though you can't use ordinary constructors, and the error messages are a little obscure at glance. https://gist.github.com/Chriscbr/b0eb99263a5d2050b7926c5fbfad2acd

In the second one (that's closer to the approach @skyrpex's outlined), all of the methods of Bucket and Function have to be defined in separate classes. https://gist.github.com/Chriscbr/b0c0d7befceb9fb5ce60a4505de28554

I think the second one might be cleaner, but the it's interesting that the single-class approach even works... (type hackery!)

@MarkMcCulloh
Copy link
Contributor Author

MarkMcCulloh commented Dec 7, 2023

all of the methods of Bucket and Function have to be defined in separate classes.

That approach is basically what I've gotten working locally, with some minor variations of the end results shown here #4842 (comment)

(An implementation note: we still need a way to access preflight fields from within inflight. So the "inflight client" still needs to expose fields from preflight)

The big question is: how important is it for the dx to match winglang (which is, theoretically, the best DX)?
It is technically possible to (eventually) get this to work without even explicitly listing the binding:

import { inflight, cloud } from "wingsdk";

const app = new cloud.App();
const bucket = new cloud.Bucket(app, "Bucket");

const fn = new cloud.Function(app, "Function", inflight(async (data) => {
  await bucket.put("hello.txt", "world");
}));

but we'll be fighting against TS and basically maintaining a second compiler on top.

I updated the description to the more TS-native approach for further discussion.

@eladb
Copy link
Contributor

eladb commented Dec 11, 2023

@MarkMcCulloh I added a Design Tenets section in the summary. Let me know what you think. Happy to discuss them!

@staycoolcall911
Copy link
Contributor

Compatible with the construct programming model used by CDKs. We will not make an effort to

@eladb your 4th design tenet is a cliff hanger :)
@MarkMcCulloh I think we should turn this into a PR, so we can converge it.

@staycoolcall911
Copy link
Contributor

@MarkMcCulloh - I feel like we can close this issue as completed. WDYT?

@eladb
Copy link
Contributor

eladb commented Feb 4, 2024

Definitely!

🚀

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Wing Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request needs-discussion Further discussion is needed prior to impl 🎨 sdk SDK
Projects
Archived in project
Development

No branches or pull requests

7 participants