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

TypeScript Support [Help Wanted] #589

Closed
willfarrell opened this issue Dec 30, 2020 · 20 comments
Closed

TypeScript Support [Help Wanted] #589

willfarrell opened this issue Dec 30, 2020 · 20 comments

Comments

@willfarrell
Copy link
Member

willfarrell commented Dec 30, 2020

TypeScript support is made up 100% from community contributions. None of the core maintainers know or use TypeScript and thus are unable to offer support for it. We believe that TypeScript has value to some and are willing to merge in changes the community find valuable.

If you would like to make Middy better, please consider submitting, reviewing, and/or commenting on PRs/issues. We generally like to give a week before merging a PR to give the community time to comment.

TypeScript Issues

@lmammino
Copy link
Member

lmammino commented Jan 3, 2021

Any reason for moving type definitions outside every project and into @types/x ?

I personally prefer to keep them colocated just to make it easier to update them every time something changes.

Am I missing something important here?

@willfarrell
Copy link
Member Author

It was more of a question on what is TS best practice. See #591
Definitions are not going anywhere.

@willfarrell willfarrell removed the v2.x label Jan 17, 2021
@willfarrell willfarrell changed the title Update TS in middy v2 [Help Wanted] TypeScript Support [Help Wanted] Jan 17, 2021
@willfarrell willfarrell removed this from the v2.0.0 milestone Jan 17, 2021
@dbartholomae
Copy link
Contributor

Best practice would be to have the type annotations right next to the code. If they are a separate file anyway, then maintaining them inside the repo in my experience works a bit better, as long as there are people who actively keep them updated and whose PRs are merged on time.
Concerning types in general, unfortunately, the setup with .use makes it hard to give really good type safety. When we last discussed improved types for this repo, I experimented around it a bit which lead to lambda-middleware. I'm still happy to help here as well, though this structural problem will limit what we can achieve.

@willfarrell
Copy link
Member Author

Thanks for commenting and bring attention to that last discussion. I've read a couple times today, will likely need another read. I'm wondering how others like express and other that also use the .use pattern have solved this TypeScript incompatibility. Would allowing middy(handler, middlewares) be all that is needed to allow better typing?

Happy to accept any help you can spare. I've updated the list at the top highlight deficiencies in v2. Just open a PR, I'm pretty active these days.

@dbartholomae
Copy link
Contributor

I'll take a look. I fear that there might be further complications, though, as TypeScript will need to know the order in which middlewares are applied, and this order only partially depends on the order in which the middleware is added.

@willfarrell
Copy link
Member Author

There are some lesser known hooks in core that might help.

  1. .use([...]) can take an array of middlewares
  2. middy(handler).__middlewares returns the arrays of the before, after, onError in their execution orders.

@KillDozerX2
Copy link
Contributor

Not to fuss much, but we are initially in the process of migrating to TS and we use typescript compiler to generate types from JS Doc and it seems to be working for some of our internal packages, maybe the core maintainers could give that a look, until someone from the community takes charge of it.

@willfarrell
Copy link
Member Author

@KillDozerX2 I tried to do that for v2, but was missing the ability to load in external custom definitions (if I remember correctly). In the end we ended up with what we have now, I far as I know it's serving peoples needs for now.

@J4YF7O
Copy link

J4YF7O commented Nov 15, 2021

Hey there ;)

I use middy just for the sqs-partial-batch-failure plugin. I don't use xray in my project. But I have to add aws-xray-sdk in my devDeps to build my app because middy is using it to describe the awsClientCapture.

Screenshot 2021-11-15 at 15 43 20

If we look at the definition of captureAWSClient in aws-xray-sdk, we realize that it's only : export function captureAWSClient<T>(service: T): T -

I'm thinking of creating a new PR to remove all aws-xray-sdk typing dependencies. Could you tell me the best place to put this definition? I was thinking of putting it in utils/index.d.ts. Is it good for you?

@willfarrell
Copy link
Member Author

@J4YF7O that sounds fine to me. However, what if aws-xray-sdk changes the definition in a future version? Ideally the *.d.ts files could could go through an esbuild like step to have its dependencies resolved on publish to not require any devDependencies. But, I'm not aware of TS supporting something like this.

@J4YF7O
Copy link

J4YF7O commented Nov 15, 2021

I clearly don't have the answer... If aws simply proposed an @types npm module for xray, that's all we could use... (and I wouldn't be here lol).

But I must lack experience to answer this question, sorry :/

@mihilmy
Copy link
Contributor

mihilmy commented Jun 7, 2022

Not sure if this is just me but using this library with typescript has been painful, even following the documentation doesn't work. I think the functionality the library provides is very powerful but as it stands makes TS usage very hard.

.handler does not exist in the types but docu specifies to use it, some of the lambda definition force an APIGW v1 vs v2, just to name a few

@willfarrell
Copy link
Member Author

@mihilmy our TypeScript support is 100% from the community. A PR to improve these areas you've identified would be more than welcomed.

@czlowiek488
Copy link

czlowiek488 commented Jun 29, 2022

I just figured out what to do to make this work well with typescript.

Problem
When you declare a function for a lambda with jsonBodyParser as middleware you are forced to type it this way.

export const handler = async (event: APIGatewayProxyEvent): Promise<APIGatewayProxyResult> => {
    const payload = event.body as unknown as { name: string };
    //...  
}

event: APIGatewayProxyEvent - other middlewares require APIGatewayProxyEvent to be typed here, however this type does not describe reality correctly. It is because after using jsonBodyParser the request.event.body is no longer a string but a object.
const payload = event.body as unknown as { name: string }; - This line overwrite a string type from APIGatewayProxyEvent to reality. And a reality is not equal to type because of jsonBodyParser middleware used there.

Solution
I belive that if we would like to make middy more typescript compatible we will need to change implementation in every place middy replaces something in APIGatewayProxyEvent.
What should be done is instead adding functionalities by replacing and extending data like below middy should just extend the original.

//jsonBodyParser middleware
  const {
    headers,
    body
  } = request.event;
  //...
  const data = request.event.isBase64Encoded ? Buffer.from(body, 'base64').toString() : body;
  request.event.rawBody = body;
  request.event.body = JSON.parse(data, options.reviver);

As you can see below, in my modified example original request data is not modified so I will be able to achieve type consistency across middlewares.

//jsonBodyParser middleware
 request.event.jsonBody = JSON.parse(request.event.isBase64Encoded ? Buffer.from(body, 'base64').toString() : body, options.reviver);

It would also decrease amount of documentation which is needed for this package because you will not need to describe how package changes aws request but only how it extend it.

Future
This would be 1 step of types improvement. After this you would be able to type like below.

export const handler = async (event: APIGatewayProxyEvent): Promise<APIGatewayProxyResult> => {
    const payload = event.jsonBody as { name: string };
    //...  
}

As you can see it is still not so good. I belive we could remove as { name: string } completely.
If we would like it to be improved even more here it what could be done.
There is a way to make type to be extended by middlewares it is moving through. However I am not good enough to implement solution since it is pretty extreme to code - in my typescript projects i use ts-pipe-compose
I belive that middy could be used this way with complete typings.

export const handler = async (event: APIGatewayProxyEvent): Promise<APIGatewayProxyResult> => {
    const payload = event.jsonBody // payload type is equal to "{ name: string }"
    //...  
}
export const handle = middy(handler)
  .use(jsonBodyParser<{ name: string }>())

Idea
I belive if type for MiddyHandler would be introduced it would make typings easier. It will give a developer of this package more control over what the typings are.
On the other hand it will make making custom typings much harder for the end user.

Also i suggest to make only one way of applying middlewares. Since now I can put middlewares into each .use or into one .use as array it will make typings quiet hard.

Using functions from ts-pipe-compose may be a way to go in order to make some part of types to be done by external library - but I am not sure it will be possible to use typings from external libraries without typescript being involved.

@dbartholomae
Copy link
Contributor

From my understanding, it is impossible to achieve this with a structure like

export const handle = middy(handler)
  .use(jsonBodyParser<{ name: string }>())

because middy(handler) can't know about any types used in .use. That's where the more TypeScript friendly approach of https://github.com/dbartholomae/lambda-middleware/ came from, first with the idea to see if we could use the approach for middy itself, and then as a separate project. There, the structure is

compose(jsonBodyParser<{name: string}>(), /* other middleware here */)(handler)

which allows a bit more type safety, as compose can ensure that its resulting function only takes a handler that is compatible with the list of middlewares. But even this approach is a bit limited.

@czlowiek488
Copy link

czlowiek488 commented Jun 29, 2022

I can agree with @dbartholomae.
It may be not possible to type it like this however, I don't have complete knowledge about typescript types.

export const handle = middy(handler)
  .use(jsonBodyParser<{ name: string }>())

I also belive a compose functions are the way to go.

@dbartholomae Would you mind telling us more about limits with approach you showed?

@willfarrell
Copy link
Member Author

Closing, but will keep pinned.

@dbartholomae
Copy link
Contributor

dbartholomae commented Jun 30, 2022

@czlowiek488 For the lambda-middleware repo, you can find the discussion here:
dbartholomae/lambda-middleware#36 (comment)

Basically, there are some limitations around generics (or were back then, I haven't checked yet with the latest TypeScript version) that limit type inference if allmiddlewares and the handler are generic. Basically if you want full type coverage, you need to write this

export const handlerWithMiddleware = middleware1()(middleware2()(middleware3()(handler)))

instead of this:

export const handlerWithMiddleware = compose(middleware1(), middleware2(), middleware3())(handler)

@vicary
Copy link
Contributor

vicary commented Jul 26, 2022

@willfarrell To make sure subsequent UseFn has a merged event type, you just need to extract the generic type from the parameters.

+ declare type MiddlewareObjEvent<T extends MiddlewareObj> = T extends MiddlewareObj<infer U> ? U : never;
+ declare type UnwrapArray<T> = T extends Array<infer U> ? U : T;

declare type UseFn<TEvent = any, TResult = any, TErr = Error, TContext extends LambdaContext = LambdaContext> =
  (middlewares: MiddlewareObj<TEvent, TResult, TErr, TContext> | Array<MiddlewareObj<TEvent, TResult, TErr, TContext>>) => MiddyfiedHandler<
- TEvent
+ MiddlewareObjEvent<UnwrapArray<typeof middlewares>> & TEvent, 
  TResult, TErr, TContext>;

EDIT: Added examples

middy(handler)
  .use(foo()) // FooEvent
  .use(bar()) // FooEvent & BarEvent
  .use(baz()); // FooEvent & BarEvent & BazEvent

@vicary vicary mentioned this issue Jul 26, 2022
34 tasks
@willfarrell willfarrell unpinned this issue Feb 26, 2023
@willfarrell
Copy link
Member Author

Please see: #1023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants