feat(core): refactor to use koa style middlewares

[nfantone]

What does this implement/fix? Explain your changes.

A PoC to let middy support Koa-style middlewares.

It also includes some (much needed, IMHO) DX improvements, namely:

• Adds eslint support through a new @middy/eslint-config shareable package (breaks current ts-standard rules).
• Adds prettier support.

Does this close any currently open issues?

Related to #622 and #626

Any other comments?

This is meant to kick off a discussion into what people expect of middy as a framework moving forward, rather than a complete or full implementation of @middy/core. As such, it doesn't really include any new documentation or tests. Not meant to be merged.

Here's how you would use it. Borrow this Gist for event.json.

'use strict'
const middy = require('middy')
const event = require('./event.json')

// Main AWS Lambda event handler
const handler = (event) => {
  return { id: event.pathParameters.id, name: event.body.name }
}

// JSON request body parsing middleware
const httpJson = () => (ctx, next) => {
  ctx.event.body = JSON.parse(ctx.event.body)
  return next()
}

// JSON response body serializer middleware
const httpResponse = () => (ctx, next) => {
  ctx.response = ctx.response.body
    ? { ...ctx.response, body: JSON.stringify(ctx.response.body) }
    : { body: JSON.stringify(ctx.response) }
  return next()
}


// Error handler middleware
const httpError = () => async (ctx, next) => {
  try {
    await next()
  } catch (err) {
    ctx.response = { body: { message: err.message }, statusCode: 500 }
  }
}

const handleEvent = middy()
  .use(httpError())
  .use(httpJson())
  .lambda(handler)
  .use(httpResponse())
  .error((err) => console.error('An unexpected error occurred', err.message))
  .handler()

handleEvent(event).then((res) => console.log(res))
// { body: '{"id":42,"name":"foobar"}' }

The new API looks pretty similar, but it does bring some changes along.

• Middlewares follow a Koa pattern for chaining: calling next dispatches the next function down the chain.
• You can interrupt the middleware chain at any point by not invoking next().
• You can catch any errors by trapping await next().
• .use() accepts a function expecting request (i.e.: the request context) and next arguments.
• .lambda() serves two purposes: wraps the handler logic inside a middy middleware and sets the execution order for the event handler within the chain, effectively discerning between "before" and "after" middlewares.
• .error() registers an optional "global" error handler for any unhandled exceptions - this is meant as a safety net for logging unexpected errors and does not replace a proper error handler middleware. Similar to koa app.on('error', err => ... ).

.error() is not a replacement for the existing onError, either. onError would be removed completely in favour of registering one or more error handler middlewares at the top of the chain.

• .handler() builds and returns the final AWS Lambda handler function. Should be called last. Similar to koa callback().

This is obviously a proposal at this stage, so I'm looking forward to hearing other people's opinion on this.

• Function names can surely be improved.
• The lambda handler placement in the middleware chain could potentially be set via other means.
• The .handler() builder call could be omitted, at the expense of composing middlewares again on each new event (would not affect cold starts).

For example, consider this alternative API that addresses points above.

middy(myHandler)
  .before(httpError())
  .before(httpJson())
  .before(cors())
  .after(httpResponse())
  .error((err) => console.error('An unexpected error occurred', err.message))

In this case, it's made clearer that the AWS handler function is a required argument for constructing a middy instance - but we lose the immediate visual grepping and ordering of the middlewares in the chain. The phases of execution are now determined by calling either .before or .after.

Todo list

• Feature/Fix fully implemented
• Added tests
• Updated relevant documentation
• Updated relevant examples

[nponeccop]

There are lint errors. Use ts-standard --fix or equivalent.

[nfantone]

@nponeccop Linter and/or tests are not meant to pass, ATM. Read the PR description, please :)

[willfarrell]

That is some pretty-looking code and has some good ideas worth exploring. My first impression is that it's too big of a change for middy to do in one step. We just got rid of callbacks in v2, adding next() back in to support koa I feel would be confusing. Keep in mind that middy is not just for http requests.

PS I can add the prettier rc file in if that helps your IDE

[nfantone]

@willfarrell

My first impression is that it's too big of a change for middy to do in one step.

100%. This is only exploratory, at the moment.

We just got rid of callbacks in v2, adding next() back in to support koa I feel would be confusing.

Would definitely need a new major release and proper docs. Not something middy really needs right now. It could potentially be published via a next channel?

I'm even playing around with the idea of offering "two cores"? Like different "middy engines" (for lack of better wording). You could keep using the current @middy/core - or swap it for @middy/core-next (awful name, I know)? Converting current middlewares to the new syntax is really straightforward so it wouldn't be hard to support both through a @middy/util wrapping function or similar. Or this could live in userland, entirely. My changes were simple enough.

PS I can add the prettier rc file in if that helps your IDE

Saw that you did that. Much appreciated.

Since we are on the topic, I'm curious: why did you decide to go with ts-standard for a non TS project? Specially since we have standard, which can be used as a standalone binary or choose the eslint equivalent eslint-config-standard and its companion plugin. Having no linter rules configured makes contributing more cumbersome than it has to be.

EDIT: Seems like eslint-config-standard did away with the plugin completely since ^16.0.0.

[willfarrell]

I'm even playing around with the idea of offering "two cores"? Like different "middy engines" (for lack of better wording). You could keep using the current @middy/core - or swap it for @middy/core-next (awful name, I know)? Converting current middlewares to the new syntax is really straightforward so it wouldn't be hard to support both through a @middy/util wrapping function or similar. Or this could live in userland, entirely. My changes were simple enough.

I think 2 versions of core would be even more confusing, which makes me think it may need to be its own project. I know a few others wrote their own core in TypeScript then use the v1.x middlewares. Happy to include it in our README if you decide to go this route.

Since we are on the topic, I'm curious: why did you decide to go with ts-standard for a non TS project? Specially since we have standard, which can be used as a standalone binary or choose the eslint equivalent eslint-config-standard and its companion plugin. Having no linter rules configured makes contributing more cumbersome than it has to be.

We started using standard for v2 alpha in leu of eslint just to clean up the number of dependencies. As you've pointed out, they're equivalent. We then switched to ts-standard for beta to support linting of the TypeScript def files.
Since my IDE defaults to standard, I didn't think about other IDEs that need a config file to let them know what to use, that's my bad. We can add one in. Thanks for bringing this up.

[nfantone]

I know a few others wrote their own core in TypeScript then use the v1.x middlewares. Happy to include it in our README if you decide to go this route.

Not really interested in re-writing the core in TS, to be honest. It's small enough as it is and the typings that we have from .d.ts files are more than enough, IMO. I'll think about publishing this as a separate module - it might be the best approach here given the timeline. Would you be interested in publishing it under a @middy scope? Or even hosting it in the same repository?

We then switched to ts-standard for beta to support linting of the TypeScript def files.

You can definitively use eslint to lint .ts files with the proper setup. Or you could just run ts-standard on .d.ts files exclusively. Having them both is absolutely fine - the number of devDependencies should not be a big issue.

Since my IDE defaults to standard, I didn't think about other IDEs that need a config file to let them know what to use, that's my bad.

Maybe we could add some doc section on how to setup the IDE if you wish to contribute, if that's the case? I'm only talking from my experience here, but a typical JS setup includes some variation of .editorconfig/prettier/eslint with their corresponding configuration files. That way, having different IDEs or setups becomes less of a hassle for everyone.

[nponeccop]

You can definitively use eslint to lint .ts files with the proper setup.
Or you could just run ts-standard on .d.ts files exclusively.

There is only one linting tool now. standard and ts-standard both use eslint. ts-standard and standard are merely command line tools to have a hassle-free config for eslint. You can do the same by configuring all eslint plugins manually. And you definitely can lint hybrid setups.

I'm for keeping the current approach (js and d.ts) for the next couple of releases. There is already much work for making proper typing for d.ts, and the core team doesn't know TS well. So TS will slow the development down. We may want it in the future for LTS releases when correctness becomes more important than rapid improvements.

[nfantone]

standard and ts-standard both use eslint. ts-standard and standard are merely command line tools to have a hassle-free config for eslint.

Sort of. They both use eslint under the hood, yes - but require a specific IDE setup and instructions to get it working (like installing this plugin on vscode, say). If that's the intention here, then it'd be nice to have that mentioned somewhere (CONTRIBUTE.md?). The way it's been used right now, it serves as an "on-demand" linter via npm scripts and nothing more. Also standard rules conflict with prettier formatting (hence the linter errors/warnings on this PR). So you might wanna address those by tweaking configuration - something you cannot do with standard, by design.

I'm for keeping the current approach (js and d.ts) for the next couple of releases.

I agree. Or maybe even all releases? Don't see any need for TS, at all, TBH.

There is already much work for making proper typing for d.ts

One approach that I have used in the past with good results is to auto-generate .d.ts typings from jsdocs using tools like tsd-docs. Paired with eslint-plugin-jsdoc, not only forces devs to add well constructed documentation, but you can forget about "typings correctness" as they'll always be properly generated. It has its caveats, but it works pretty well when done right.

[nponeccop]

Also standard rules conflict with prettier formatting

This defeats the purpose of standard. It comes with its own, well, standard formatting through --fix, so if you buy into standard at all, you should drop prettier and stick to whatever standard wants.

but require a specific IDE setup and instructions

They don't. Eslint and typescript work exactly the same way from npm run lint. And all setup you need can be done once using package.json and .eslintrc.yml. There is a relatively easy way to make eslint use standard rules.

Eslint IDE support is a separate thing from command line, and Standard is sometimes supported too. Standard setup is marginally easier than the lint way of course, and doesn't pollute your package.json devDependencies.

Or maybe even all releases? Don't see any need for TS, at all, TBH.

TS has an insane level of support from JetBrains. So unless you are a fan of JetBrains/IntelliJ IDEs there is no point.

is to auto-generate .d.ts typings from jsdocs

Nah, that would generate mediocre typings, especially for such framework as middy, which does a lot of dynamic things such as modifying the event object. You can only do that if you don't care about TS at all. Middy does need good typings with generics, intersection types and other TS type-fu, which reflect the manipulations performed by particular middleware, in order to be useful in an instrumented setting (that is, realistically in any TS setting). I hope to provide very good typings when I start using middy. Which is hopefully in next couple of weeks.

[nfantone]

I feel like we are digressing from the main point of focus of this PR, so I'll only briefly address your comments above, @nponeccop.

This defeats the purpose of standard [...] you should drop prettier and stick to whatever standard wants.

Not really the case, IMHO. Of course, it all boils down to a matter of preference. I see prettier as my formatter of choice and standard as a set of rules. Both can live happily side by side, no purpose defeated. Ironically, eslint and/or prettier are much more... erhm... standard than standard. You'll be pleasing a wider audience. This is relevant in a public repository of an open source library.

They don't. Eslint and typescript work exactly the same way from npm run lint.

They do. And no. It's super easy to confirm: I cloned the repository, opened my IDE and got no linting feedback or auto-formatting whatsoever. No git hooks, either - so at no point in my contribution development my code was check for style locally. Not sure where you are coming from.

Nah, that would generate mediocre typings, especially for such framework as middy, which does a lot of dynamic things such as modifying the event object.

From your comments, I reckon you have never used the tools I mentioned. You should give it a try before dismissing it or forming an opinion. The typings are as good as your jsdocs. If your docs are good, your typings will follow. Not sure what "TS-Fu" you are specifically referring to (generics and intersections are both pretty basic and fully supported in jsdocs), but you can always manually tweak typings once generated, if really needed.

[rsdavis]

Hey guys, thanks for your work on this project. I've only been using middy briefly, but I wanted to share a few thoughts. Please just let me know if I'm off base here.

1. The handler is always last

It's been awhile since I've used Koa, but I think this is in keeping with its design. It seems weird to me that you would specify the the order of the handler within the middleware stack using something like .lambda(handler). For example, the httpResponse middleware was called before the handler in the example above. But thats not necessary. If a middleware needs to run after the handler, then take advantage of the design by awaiting the handler to finish, then grab the response as follows ...

// JSON response body serializer middleware
const httpResponse = () => async (ctx, next) => {
  await next()
  ctx.response = ctx.response.body
    ? { ...ctx.response, body: JSON.stringify(ctx.response.body) }
    : { body: JSON.stringify(ctx.response) }
}

const handleEvent = middy()
  .use(httpError())
  .use(httpJson())
  .use(httpResponse())
  .use(handler)

2. Everything is middleware, even the base handler

In the example posted at the top of this thread, a .handler() function is called at the end of the stack I guess to signify done. I don't think this is necessary. With the handler being last, it just becomes another layer of middleware. The only difference is that it's not a function that returns a function. It's just a function that doesn't call next.

const handleEvent = middy()
  .use(httpJson())
  .use(async (event, next) => {
      // here is my base handler
      // next is not called 
   })

3. ctx === event

I understand that ctx is used here in inspiration from koa. But, I just want to confirm that the ctx is the event. Maybe this is the intention. With the handler being middleware, it makes sense that every middleware would see the exact same event object. It just gets modified as it travels down (and back up) the stack.

On the other hand, if for some reason ctx needs to be something else, then the handler should also receive ctx as an argument, and then access the event using ctx.event. In other words, I can't see the logic in the handler receiving something different from other middlewares.

4. Maybe new project

I think it's been mentioned a couple times already that this is a large refactor of how middy works. And reading your posts above, it seems that many are maintaining alternate versions of the middleware anyway. So, I'm putting in my vote for starting a new project. Oddly enough, that would reflect the history of the Express -> Koa transition.

By the way, let me know if this discussion is now happening elsewhere. Also, I'm happy to help where I can :D

[willfarrell]

Looks like @vendia/serverless-express has been gaining lots of traction over the last year and says it can handle koa middlewares. I still don't see middy supporting middlewares from other frameworks in the near future. Closing for now.