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

DAG of Middleware Dependencies #498

Closed
baileytincher opened this issue Apr 1, 2020 · 9 comments
Closed

DAG of Middleware Dependencies #498

baileytincher opened this issue Apr 1, 2020 · 9 comments

Comments

@baileytincher
Copy link
Contributor

Let me provide a sample use case. I have a bunch of middlewares, some dependent on each other, some dependent on nothing. For example, I use a JSON body parser, schema validator, and a database connection middleware.

async Connection Middleware
Body parser -> schema validator

I don't necessarily need the database connnection middleware to finish before the body parser/schema validator, but I do need the promise to resolve before getting to the function. I can hack this by setting the connection as a variable on the event and then adding a final middleware that actually does the await on this. However, this is not necessarily clean.

Has any thought been given to specifying middlewares as a directed-acyclic graph, or would a PR be considered for this if not?

@willfarrell
Copy link
Member

Thanks for posting. This is something I've thought about. There are theoretical performance gains to be had. I'd be open to a PR and/or discussion on this. Curious what ideas you have in mind to accomplish this.

@willfarrell
Copy link
Member

I've started working on v2. Where I'm currently at is support better async support is unifying middleware the make external requests. Some patterns I'm inforcing:

  • always prefetch request during cold start when possible
  • store response promises to the handler.context
  • await response only when it's needed

Going full DAG would be pretty sweet, however I feel it would get way too complicated, the orchestration overhead may cancel out the performance benefits that would be gained compared to the much simpler above approach. If you know of a js lib that does this well I'll take a look.

@baileytincher
Copy link
Contributor Author

baileytincher commented Dec 29, 2020

Your idea sounds like the right approach.

However is the context the right place to store it? That would break type compatibility with the standard AWS request context and seems like it would be messy for TS users to specify.

Not sure if you have any objections to this, but since middy already specifies handler.error as a custom attribute then perhaps it makes sense to expand that to a handler.middlewareContext for example

@willfarrell
Copy link
Member

Hmmm, we already do this in other middlewares now, just not consistently. Storing it elsewhere as you suggested is easy enough to do, but if a TS user wants access to the values they requested in the middlewares (ie ssm), they'll have the same problem if we stored it in context.

@baileytincher
Copy link
Contributor Author

As a TS user I've had to create wrappers around middy for that reason. The introduction of generics into the library would be a huge help to avoid that and could solve the problem you mentioned.

As an example I have a (very Work In Progress) library I made for myself for the sake of type generics on things like the event body, headers, etc. that allows for usage like https://github.com/ShallotJS/Serverless-REST-TypeScript-Starter/blob/ade627179644953586fae97de7aeaaa787ed05e7/src/index.ts#L11

That uses what was effectively my middy clone but for typescript (the docs are extremely sparse still) https://github.com/ShallotJS/shallot

@willfarrell
Copy link
Member

Thanks for this super useful insight in to TS users. Clearly I'm not a TS user. I'll take this into consideration as I progress further. Maybe handler.internal, then provide and example on how to copy what is needed over to handler.context. I assume you can extend existing definitions easily to accommodate this use case?

@willfarrell
Copy link
Member

This approach also solves the need to add in flags to clear secrets manually. Liking more and more. Thanks again.

@baileytincher
Copy link
Contributor Author

I'm not very experienced with TS-Doc comments, but it should be possible. I used things like Omit to allow more exact types in my handlers with Middy like so https://pastebin.com/32jKbMZ1

If you have an issue/branch that you are working on this I'm happy to help out where you need it!

@willfarrell
Copy link
Member

Thanks, I will definitely take you up on that. I have an open PR #587 to release/2.x. When that gets merged I'll be ready to for some TS def contributions.

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

No branches or pull requests

2 participants