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

Latest version of @middy/core breaks types in the handler function #1221

Closed
vidarc opened this issue Jun 27, 2024 · 9 comments
Closed

Latest version of @middy/core breaks types in the handler function #1221

vidarc opened this issue Jun 27, 2024 · 9 comments
Labels

Comments

@vidarc
Copy link

vidarc commented Jun 27, 2024

Describe the bug
The 5.4.2 release of @middy/core breaks typing that works fine in version 5.4.1. Have tried it with TypeScript 5.4.x and 5.5.x and encounter the same issue.

To Reproduce
You can see this issue with the sample TypeScript in the documentation. The req and context variables in the handler function are now no longer correctly typed, but instead return as any. Downgrading back to 5.4.1 fixes the issue and now req and context are correctly typed again.

import middy from "@middy/core";
import secretsManager from "@middy/secrets-manager";
import type { APIGatewayProxyEvent, APIGatewayProxyResult } from "aws-lambda";

export const handler = middy<APIGatewayProxyEvent, APIGatewayProxyResult>()
  .use(
    secretsManager({
      fetchData: {
        apiToken: "dev/api_token",
      },
      awsClientOptions: {
        region: "us-east-1",
      },
      setToContext: true,
    }),
  )
  .handler(async (req, context) => {
    // The context type gets augmented here by the secretsManager middleware.
    // This is just an example, obviously don't ever log your secret in real life!
    console.log(context.apiToken);
    return {
      statusCode: 200,
      body: JSON.stringify({
        message: `Hello from ${req.path}`,
        req,
      }),
    };
  });

Expected behaviour
Above example should work with TypeScript

Additional context
Add any other context about the problem here.

@vidarc vidarc added the bug label Jun 27, 2024
@tim-helloquickly
Copy link

I'm also experiencing this!

@naorpeled
Copy link
Contributor

naorpeled commented Jun 30, 2024

Hey,
you're right, the type defs indeed changed.
Wasn't sure how to properly document this and am sorry for the inconvinience.

What you need to do is to provide the generic type to the .handler function.
In your case it would be:

import middy from "@middy/core";
import secretsManager from "@middy/secrets-manager";
import type { APIGatewayProxyEvent, APIGatewayProxyResult } from "aws-lambda";

export const handler = middy()
  .use(
    secretsManager({
      fetchData: {
        apiToken: "dev/api_token",
      },
      awsClientOptions: {
        region: "us-east-1",
      },
      setToContext: true,
    }),
  )
  .handler<APIGatewayProxyEvent, APIGatewayProxyResult>(async (req, context) => {
    // The context type gets augmented here by the secretsManager middleware.
    // This is just an example, obviously don't ever log your secret in real life!
    console.log(context.apiToken);
    return {
      statusCode: 200,
      body: JSON.stringify({
        message: `Hello from ${req.path}`,
        req,
      }),
    };
  });



// OR
import middy from "@middy/core";
import secretsManager from "@middy/secrets-manager";
import type { APIGatewayProxyEvent, APIGatewayProxyResult } from "aws-lambda";

export const handler = middy<APIGatewayProxyEvent, APIGatewayProxyResult>(
async (req, context) => {
    // The context type gets augmented here by the secretsManager middleware.
    // This is just an example, obviously don't ever log your secret in real life!
    console.log(context.apiToken);
    return {
      statusCode: 200,
      body: JSON.stringify({
        message: `Hello from ${req.path}`,
        req,
      }),
    };
  }
)
  .use(
    secretsManager({
      fetchData: {
        apiToken: "dev/api_token",
      },
      awsClientOptions: {
        region: "us-east-1",
      },
      setToContext: true,
    }),
  );

@willfarrell
Copy link
Member

Please reopen if this needs more attention.

@jlarmstrongiv
Copy link

Related discussion in #1217

@naorpeled
Copy link
Contributor

Related discussion in #1217

Will be able to further investigate in few hours, will keep you posted. Sorry for the inconvenience.

@naorpeled
Copy link
Contributor

Hey guys,
first of all, sorry for the inconvenience.

I've created a PR to partially revert those changes,
need to figure out how to support plain AWS Lambda types (such as S3Handler) properly.

If anyone has an idea please let me know 🙏

@jlarmstrongiv
Copy link

jlarmstrongiv commented Jul 1, 2024

@naorpeled wouldn’t it look something like:

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

export const middy = middyCore<S3Event, void>();

export const handler = middy.handler(({ Records }) =>
  console.log(JSON.stringify(Records, undefined, 2))
);

Must be version 5.4.1 though.

The key with the previous middy types is focusing on middyCore<requestOrEvent, responseOrReturnType, optionalError, optionalContext, tInternal>.

So you don’t want the S3Handler type. But if you look at its definition:

export type S3Handler = Handler<S3Event, void>;

You’ll find the types you want—S3Event as the event and void as the return type. You’d specify it like above with middyCore<S3Event, void>();

The main problem I have with middy types is that middy’s default middleware augments types with APIGatewayProxyEvent, despite me specifying APIGatewayProxyEventV2. That means my actual middy handler types include properties from APIGatewayProxyEvent, despite them not existing in the HttpApi event APIGatewayProxyEventV2.

@iainlane
Copy link

iainlane commented Jul 3, 2024

Could this be reopened please?

Here's something which breaks with 5.4.2 for me (a simplified version of our codebase).

We have handlers which return objects satisfying an internal interface, and then middleware to transform from that into an APIGatewayProxyResponseV2:

import middy, { MiddlewareObj } from "@middy/core";
import type {
  APIGatewayProxyEventV2,
  APIGatewayProxyResultV2,
  Context,
} from "aws-lambda";

export interface LoggerContext extends Context {
  logger: unknown;
}

// Adds a logger to the context
export function loggerMiddleware<TEvent, TResult>(): MiddlewareObj<
  TEvent,
  TResult,
  Error,
  LoggerContext
> {
  return {
    before: ({ context }) => {
      context.logger = undefined;
    },
  };
}

// Converts from our type to an AWS response type
export function renderableMiddleware<TErr>(): MiddlewareObj<
  APIGatewayProxyEventV2,
  APIGatewayProxyResultV2 & MyType,
  TErr,
  LoggerContext
> {
  return {
    after: (request) => {
      if (request.response === null) {
        return;
      }

      // Imagine some logging here making use of `request.context`.

      Object.assign(request.response, {
        statusCode: 200,
        body: `Got ${request.response.apples} apples`,
        headers: {
          "Content-Type": "application/json; charset=utf-8",
        },
      });
    },
  };
}

interface MyType {
  apples: number;
}

function baseHandler(): MyType {
  return {
    apples: 1337,
  };
}

const handler = middy()
  .use(loggerMiddleware())
  .use(renderableMiddleware())
  .handler(baseHandler);

// With 5.4.1, type is `Promise<any>`.
// With 5.4.2, type is `Promise<MyType>`. #1222 doesn't change this either :(
const result = handler(
  {} as APIGatewayProxyEventV2,
  {} as LoggerContext & Context,
);

The types weren't the best before - it would be nice if after and before functions could talk about how they transform their inputs. The ideal type for result would be Promise<APIGatewayProxyResultV2> and I'd assign the response directly instead of using Object.assign.

But given the existing type setup, a type that would work and the one I'd expect to see would be Promise<APIGatewayProxyResultV2 & MyType>.

Promise<MyType> isn't right, and it breaks the types in my codebase, unfortunately.

@iainlane
Copy link

iainlane commented Jul 5, 2024

I couldn't resist fiddling with the type definitions a bit. I came up with this: a66f358.

In there we add a couple of new generic parameters which exist to propagate along the types from after() and before(), and modify the type of handler().

It's not a proposal right now. Partly because it breaks existing programs and partly because I'm not sure it's all correct. It does seem to improve things for me, though.

I wanted to share it to get feedback and see if folks thing it's a valuable direction to try to move in.

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

No branches or pull requests

6 participants