dbartholomae / lambda-middleware

A collection of middleware for AWS lambda functions.
https://dbartholomae.github.io/lambda-middleware/
MIT License
151 stars 18 forks source link

Unable to JSON serialize type with optional keys #43

Closed parkan closed 3 years ago

parkan commented 3 years ago

Describe the bug

The json serializer works fine for object literals, however if passed a type with optional members (I generally return models from my handlers, which usually have optional fields), it doesn't typecheck.

To Reproduce

  describe("with a handler returning a type with optional members", () => {
    let response: any;

    type Thing = {
        id?: string
        foo: string
    }

    beforeEach(async () => {
      const handlerResponse: Thing = { foo: "bar" };
      const handler = async () => handlerResponse;
      const handlerWithMiddleware = jsonSerializer()(handler);
      response = await handlerWithMiddleware(createEvent({}), createContext());
    });

    it("returns 200", async () => {
      expect(response).toMatchObject({ statusCode: 200});
    });

    it("returns serialized non-optional members", async () => {
      expect(response.body).toEqual("{\"foo\":\"bar\"}");
    });
  })

yields

    src/JsonSerializer.test.ts:38:54 - error TS2345: Argument of type '() => Promise<Thing>' is not assignable to parameter of type 'PromiseHandler<APIGatewayProxyEvent, { [key: string]: JSONPrimitive; } | JSONObject[] | undefined>'.
      Type 'Promise<Thing>' is not assignable to type 'Promise<{ [key: string]: JSONPrimitive; } | JSONObject[] | undefined>'.
        Type 'Thing' is not assignable to type '{ [key: string]: JSONPrimitive; } | JSONObject[] | undefined'.
          Type 'Thing' is not assignable to type '{ [key: string]: JSONPrimitive; }'.
            Property 'id' is incompatible with index signature.
              Type 'string | undefined' is not assignable to type 'JSONPrimitive'.
                Type 'undefined' is not assignable to type 'JSONPrimitive'.

Expected behavior

Type check passes as the optional key is still of the right type.

Additional context

This appears to be a bit of a typescript wart, as optional members are treated as equivalent to having | undefined under strict null checks, which doesn't really make sense to me given that even in underlying JS hasOwnProperty can distinguish the two cases.

This seems like it can be fixed by redefining type JSONPrimitive = string | number | boolean | JSONObject | undefined;, though this isn't technically valid JSON, JSON.stringify has a relatively defined behavior for it.

I think there is a way to do this cleanly with keyof/Pick, trying to work it out...

EDIT: I should also note that this doesn't work at all if the returned value is typed as interface; that seems to really break the index signature inference

parkan commented 3 years ago

Ok, I am not quite able to make the mapped type work here, not really sure how to apply it to the type parameter on JSONObject, what I have so far is

type RequiredKeys<T> = { [k in keyof T]-?: undefined extends T[k] ? never : k }[keyof T];
type PickRequiredKeys<T> = Pick<T, RequiredKeys<T>>

but I'm not sure how to use it in the PromiseHandler signature

EDIT: ok, I am able to get unit tests to pass with

type RequiredKeys<T> = { [k in keyof T]-?: undefined extends T[k] ? never : k }[keyof T];
type PickRequiredKeys<T> = Pick<T, RequiredKeys<T>>
export const jsonSerializer = () => (
  handler: PromiseHandler<APIGatewayEvent, PickRequiredKeys<JSONObject> | undefined>

However, using a mapped type on the type bound in that signature is bending my brain a little bit so I'm not 100% sure it's correct

parkan commented 3 years ago

BTW thanks for being so responsive on these libraries, it seems like you're the only maintainer so I really appreciate it as it as they really fit my usecase and I'm frankly surprised they are not more widely used. This isn't strictly as "bug" as much as a limitation, but there was no better template to use. I'm happy to PR with these changes if they make sense to you, I'm still relatively new to typescript (mostly Scala background for this kind of thing) so I am not entirely sure of myself and appreciate the guidance.

dbartholomae commented 3 years ago

Thanks! I've quickly fixed it myself, as the setup with Rush currently is a bit finicky.

dbartholomae commented 3 years ago

It's released in version 2.1.0 https://www.npmjs.com/package/@lambda-middleware/json-serializer