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

feat: self-composing UseFN #883

Merged
merged 22 commits into from
Aug 13, 2022
Merged

Conversation

vicary
Copy link
Contributor

@vicary vicary commented Jul 31, 2022

In respond to #769 (comment), this is the first attempt to make the event object self- and auto- composing through the chain of .use().

Intended usage:

import type middy from "@middy/core";
import type { Context } from "aws-lambda";

const middyFoo = (): middy.MiddlewareObj<{ evtFoo: string }, any, Error, Context & { ctxFoo: string }> => ({
  before: ({ context, event }) => {
    context.ctxFoo = "foo";
    event.evtFoo = "foo";
  },
});

const middyBar = (): middy.MiddlewareObj<{ evtBar: string }, any, Error, Context & { ctxBar: string }> => ({
  before: ({ context, event }) => {
    context.ctxBar = "bar";
    event.evtBar = "bar";
  },
});


export const handler = middy()
  .use(middyFoo())
  .use(middyBar())
  .before(({ context: { ctxFoo, ctxBar }, event: { evtFoo, evtBar } }) => {
    // context and event inherits new properties from the .use() above
  })
  .handler(({ evtFoo, evtBar }, { ctxFoo, ctxBar }) => {
    // context and event inherits new properties from the .use() above
  });

@vicary
Copy link
Contributor Author

vicary commented Aug 1, 2022

@willfarrell Should it be 3.0 or 4.0?

Also some old plugins add something to lambda context, while others add to events. I am kinda leaning towards extending the event object, so I intentionally left TContext untouched. Do you have a preference or encourage either side?

@willfarrell
Copy link
Member

v4 is on hold till we know what AWS is going to do for their next runtime update (v18). That could be as early as Sept, or a year from now. I'm open to minor changes for v3 for now. There can also be a separate PR for v4, to allow others to comment.

Changes to the event are the most common use in middlewares, however non-external inputs (ie secrets, db connections, etc) should live in the context.

I personally don't use TS, so it's up to the community to decide what is best for Middy. This PR looks safe fro v3.

@vicary
Copy link
Contributor Author

vicary commented Aug 1, 2022

My team is currently adding everything to event, including typeorm and fetch, to make handlers testable.

But I want this PR to be generally aligned with your advertised usage, I'll update it again to have TContext also inherits through the chain of .use().

@vicary vicary force-pushed the feat-autocompose-use branch 2 times, most recently from 380d1d4 to 2b39e42 Compare August 1, 2022 15:07
@vicary
Copy link
Contributor Author

vicary commented Aug 1, 2022

@willfarrell It's ready now.

Tried avoiding the necessity of Context & ... in usage, but that breaks the extends LambdaContext check. I chose to retain a more guaranteed typing.

Usage example in the description is updated to reflect the change.

@vicary
Copy link
Contributor Author

vicary commented Aug 2, 2022

We may update the types in all packages, shall I do that here or in a new PR?

For example, @middy/cloudwatch-metrics with typings:

export const handler = middy()
  .use(cloudwatchMetrics())
  .handler(({}, { metrics }) => {
    // The `metrics` property and its methods will be discoverable by IDEs
  });

@willfarrell
Copy link
Member

Within this PR is fine.

@vicary
Copy link
Contributor Author

vicary commented Aug 2, 2022

@willfarrell added types for middlewares, please comment.

@vicary
Copy link
Contributor Author

vicary commented Aug 2, 2022

TypeScript has no way to perform string matching without introducing unused vars, I have to skip these errors.

// eslint-disable-next-line @typescript-eslint/no-unused-vars
type ExtractSingles<T> = T extends `/${infer _}` ? never : T

// eslint-disable-next-line @typescript-eslint/no-unused-vars
(keyof TOptions['fetchData'] extends `${infer _P}/${infer _S}`

@vicary
Copy link
Contributor Author

vicary commented Aug 3, 2022

Took another look at SSM and noticed a mistype, fixed it.

Sidenote: ts-standard is broken

ts-standard (npm run test:lint:ci) is not suitable for typescript linting because there are broken rules, for example typescript-eslint/typescript-eslint#1824.

I think we should use eslint directly so we can opt-in and out of particular rules, along with eslint-plugin-prettier so contributors can easily fix trivial format misalignments.

That's for another PR though, I'll stick with the broken indentations here to stay green.

@vicary
Copy link
Contributor Author

vicary commented Aug 5, 2022

Rebased

@willfarrell
Copy link
Member

Looks good to me. I'm going to leave it open in case other would like to comment. I can merge and release next week. Thanks for all of the hard work putting this together.

@willfarrell willfarrell merged commit 8d9bd13 into middyjs:main Aug 13, 2022
@vicary vicary deleted the feat-autocompose-use branch August 13, 2022 12:36
@bilalq bilalq mentioned this pull request Sep 6, 2022
34 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants