-
-
Notifications
You must be signed in to change notification settings - Fork 376
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
Version 4 #769
Comments
@willfarrell Does My team is currently repeating the following style in hundreds of resolvers. Every night, I dream of making all the middleware events private and self-contained. const resolver = async (event: FooEvent & BarEvent & BazEvent) => { ... };
export const handler = middy(resolver).use(...).use(...).use(...); |
@vicary this will be in v3. What happens is whatever is passed into It means you can do Which mean it's easier to split up repeated middlewares:
Technically you can do this in v2.x but you have to wrap it in a function, which isn't as easy to use. |
@willfarrell Not sure if I get it. I specifically wants to remove the necessity to chain import type { AppSyncResolverEvent } from "aws-lambda";
export const handler = middy()
.use(foo()) // does TEvent & FooEvent inside
.use(bar()) // does TEvent & BarEvent inside
.use(baz()) // does TEvent & BazEvent inside
.handler<AppSyncResolverEvent>((event) => {
// Can I automatically get all the followings?
event.foo; // type FooEvent = { foo: number; [key: string]: any };
event.bar; // type BarEvent = { bar: string; [key: string]; any };
event.baz; // type BazEvent = { baz: PartialDeep<BaseEntity>; [key: string]: any };
event.arguments; // AppSyncResolverEvent
}); |
middy is just a light framework to order repeated operation before/after a handler execution. What is in those middlewares is completely up to you to fit your needs. All of the following are the same:
Hope that help in your planning on how to best use middy. Your above example looks like it will run just fine in v3. If you move the handler into |
@willfarrell Main problem is TypeScript implementation does not carry/merge event types down the chain, it should be possible in the DT for My second code won't pass type checking because the handler type has no knowledge about I don't want to manually import all the intermediate middleware types in my handler |
Ooohhhh, this is a typescript questions. Sorry. I personally don't use typescript so I can't advise how to best use middy with typescript. I know you're not the first to run into this issue, I'm unsure how other have resolved this. I'm open to feedback on how to this can be improved within middy, if even possible. |
It's mildly troubling, but somehow breaking how types work. Not sure if I have the time and energy to whip up a PR for the already launching v3, so I am speaking in this v4 thread. |
v3 is only in early alpha, it wont release until after node v16 is supported in lambda (likely Apr). So lots of time to discuss any breaking changes, especially for TypeScript. Otherwise they'll have to wait a year. |
That's great! I'll see what I can do next month. |
@vicary You might want to take a look at https://github.com/dbartholomae/lambda-middleware which I created out of a discussion around how to better use TypeScript types in middy. It attacks the problem by using functional composition, so middlewares can define their input and output types. |
@dbartholomae Looks great - I recently recreated the middleware pattern for TypeScript too (https://github.com/evilkiwi/lamware), but went a different route with chaining |
I love the documentation and ideas behind middy, but the TypeScript developer experience is broken right now. The middleware implementations that @dbartholomae and @oyed put out look great, but now we have a problem of fragmentation. Middy has the biggest body of plugins, but TypeScript usage is dangerously unsafe. I see that the lambda-middleware package has a helper for adapting middy-based ones, but it's still a hassle to have to manually type that all out. It'd be much nicer if the internal types of middy middlewares themselves behaved more like the other two middleware orchestrators. On a loosely related note, it'd be great to use named exports only and drop the default exports. The benefit of this isn't limited to just TypeScript users, since VSCode intellisense completion works pretty well for JS as well. This would be a breaking change, but it should improve the developer experience a fair bit. |
@bilalq Thanks for the TS feedback. None of the core maintainers know TS, so all of our defs are community generated. If someone from the TS community would like to audit and update all middleware in a PR, I'd be happy to review. We can easily have named exports alongside the default without a breaking change if it makes that big of a difference for developers. I would like to better understand why you think this would improve the developer experience before making this change. Is there an article / blog post you can share explaining this? |
@willfarrell I can start from what I know, if you're OK with the approach in #589 (comment). In the same PR I'll also fix inline middlewares, because they are broken in a way that their |
I look forward to reviewing the PR. |
Shouldn't it be |
Yeah, TypeScript is important for me, and the fact its poorly implemented here is why I have chucked the last two days work trying to refactor to use Middy and am searching for a more competent solution. |
@codyfrisch Sorry to hear Middy TypeScript definitions are not meeting your needs. A PR to help improve them is welcome if you'd like to contribute back to the community. I've talked with other OSS maintainers about supporting TypeScript well, nearly all are also struggling with it. This article was shared with me that I hope will shed light on this topic: https://erock.prose.sh/typescript-terrible-for-library-developers |
@willfarrell FWIW, the #883 improved the situation a ton. Really appreciate the work you and @vicary did to get that merged in! Honestly, at this point, I have no reservations about using Middy with TS. It's been pretty stellar. Regarding your point about difficulty in supporting TypeScript as a library author: I agree that it is annoying if your source code itself isn't authored in TypeScript. However, I've found maintaining libraries and frameworks a lot easier when they're actually written in TS compared to what it was like back in the pure JS days. The article you linked to is definitely right about how the heavy need for complex type expressions in libraries vs end apps basically forces you to build up expertise on the type system if you want to have a smooth developer experience. |
@bilalq To be fair, it's impractical to overhaul a project if the authors are not already using TypeScript everyday. But if refactoring is happening by any chance, count me in for the help and long term maintenance. |
Hey @willfarrell, |
@naorpeled Thanks for the offer! I've been rethinking whether to include this in v4 or just add to v3. Depending on how it's implemented I think it could be included into v3 as a minor update and not have to wait for v4 to release (date unknown - waiting on AWS). I was imagining it would parse by default with an option to disable for APIG use. Please feel free to open a PR for this off of the |
From a feature standpoint, #908, is ready for an alpha release. Because NodeJS 18 runtime has not released yet, additional change may be applied later. Please take a look and give feedback. If someone could review and update the TypeScript to accommodate the changes to support AWS SDK v3, that would be greatly appreciated. Thanks |
Hey, Regarding the task we've discussed earlier here, I did some digging into the event payloads in the different AWS services but I feel I need to look further into it, was on a spontaneous vacation so didn't have time to look further into it, I think I'll be able to work on it during this weekend. |
@naorpeled No worries, we all need vacation. I already knocked out the feature for parsing pathParams. I woke up with an idea on how to do it cleanly, so I just did it. It just needs to be released, something for tomorrow. |
👑 |
an initial alpha is now released for testing |
nodejs18.x runtime is finally here. v4 will release soon after some additional testing. |
PR: #908
Planned
engines
to"node": ">=16"
node-version
swc
to usees2021
(https://github.com/tsconfig/bases/tree/main/bases)event-normalizer
: review for any new events from AWS docs and event listhttp-security-headers
: review against helmet, MDN, ASVSssm
,secrets-manager
, etc (https://aws.amazon.com/blogs/developer/pagination-using-async-iterators-in-modular-aws-sdk-for-javascript/)http-urlencode-body-parser
throw 422 when unable to parsehttp-content-encoding
pull stream piping into new middlewarestream-transform-response
http-router
parse path params. no, can be done in v3http-router
REST and HTTP route paths should be the same (http-router needs stage name in path for routes declaration #873). No, see issue.validator
exportcompileSchema
and require it be passed in to allow for better tree-shaking.use(await middleware())
>= 15
event-normalizer
[skip - slower than current]remove polyfills
core
AbortControler
polyfilltimers/promises
polyfillhttp-content-encoding
: removestream/promises
polyfillutil
update tests to usetimers/promises
for delay>= 16
Error {cause} >= 16.9
util
updatecreateError
to useclass HttpError extends Error
http-json-body-parser
: use propernew Error(message, {cause})
notationhttp-multipart-body-parser
: use propernew Error(message, {cause})
notationutil
: use propernew Error(message, {cause})
notationutil
: use propersuper(message, {cause})
notation inHttpError
validator
: use propernew Error(message, {cause})
notationdeprecate
cloudwatch-metrics
for@aws-lambda-powertools/metrics
(uses EMF)? @fredericbartheletvalidator
: deprecateinputSchema
andoutputSchema
optionsevent.rawBody
inhttp-json-body-parser
The text was updated successfully, but these errors were encountered: