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

Maintenance: add mechanism to check compatibility with @types/aws-lambda package #1818

Open
1 of 2 tasks
am29d opened this issue Dec 13, 2023 · 1 comment
Open
1 of 2 tasks
Labels
discussing The issue needs to be discussed, elaborated, or refined internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) parser This item relates to the Parser Utility

Comments

@am29d
Copy link
Contributor

am29d commented Dec 13, 2023

Summary

When using our built-in schema with zod, developers can infer the types with type SqsEvent = z.infer<typeof SqsSchema> and use it in their Lambda handler. Another popular option is to use @types/aws-lambda package wich has 3M downloads per week. We need to make sure that both types are compatible, so there is no confusion or unexpected behaviour.

Why is this needed?

If either @types/aws-lambda or our schema is updated, we want to have a reliable test to verify that they are both compatible and if not to fix one them.

Which area does this relate to?

Parser

Solution

I have checked the first idea that worked:

import { z } from 'zod';
import { ALBEvent, APIGatewayProxyEvent } from 'aws-lambda';
import { AlbSchema } from '../../src/schemas/alb';
import { APIGatewayProxyEventSchema } from '../../src/schemas/apigw';

type AlbType = z.infer<typeof AlbSchema>;
type ApiGatewayType = z.infer<typeof APIGatewayProxyEventSchema>;
describe('Types compatibility', () => {
  it('ALBEvent types are compatible', () => {
    // These functions will fail to compile if A and B are not structurally identical
    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    const A = (value: AlbType): ALBEvent => value;
    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    const B = (value: ALBEvent): AlbType => value;
  });

  it('ApiGatewayEvent types are compatible', () => {
    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    const A = (value: ApiGatewayType): APIGatewayProxyEvent => value;
    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    const B = (value: APIGatewayProxyEvent): ApiGatewayType => value;
  });
});

The compiler will fail if the structure does not match and will tell exactly which properties are not compatible.

Alternatively research more about https://github.com/fabien0102/ts-to-zod. If we can convert @types/aws-lambda to schemas, we might directly compare the schemas to find the differences.

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

@am29d am29d added triage This item has not been triaged by a maintainer, please wait internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) labels Dec 13, 2023
@am29d am29d added this to the Parser - Beta Release milestone Dec 13, 2023
@dreamorosi dreamorosi added parameters This item relates to the Parameters Utility discussing The issue needs to be discussed, elaborated, or refined and removed triage This item has not been triaged by a maintainer, please wait labels Dec 13, 2023
@am29d am29d added the good-first-issue Something that is suitable for those who want to start contributing label Dec 18, 2023
@am29d am29d added parser This item relates to the Parser Utility and removed parameters This item relates to the Parameters Utility labels Jan 3, 2024
@dreamorosi dreamorosi removed this from the Parser - Beta Release milestone Feb 21, 2024
@dreamorosi dreamorosi removed the good-first-issue Something that is suitable for those who want to start contributing label Mar 6, 2024
@dreamorosi
Copy link
Contributor

Copying this comment I have left under a PR since it's relevant to the conversation:


[...] I think we should agree & articulate our stance on this topic a bit better to avoid future confusion amongst us and towards customers.

The main point here, besides calling out potential discrepacies between us & the other library, is in my opinion to establish our strategy towards event types as a whole. This will then influence which actions we'll take now and in the future, as well as when discrepacies are detected.

Currently the @types/aws-lambda types are community maintained and manually authored. AWS events can change and the types can contain issues.

While I agree with the sentiment of maintaining these types and our schemas (and in the future JSON schemas as well) up to date and aligned, I don't think this ultimately guarantees that the types/schemas are actually correct.

As a think big idea, we should at least consider the option of instead investing efforts in creating automations to periodically test against real AWS events to get the actual shapes of the events sent to Lambda. Based of those, we could then derive the schemas as well as the TypeScript types.

Once we detect drift (aka changes in the events), we could then propagate them to the appropriate destinations be it within the scope of Powertools or @types/aws-lambda.

I am not expecting the whole industry to move over our own types (i.e. @aws-lambda-powertools/events or @aws-lambda-powertools/types) but I would still aim at becoming the source of truth for these events/schemas/types.

The DefinitelyTyped repo (aka the one that hosts @types/* packages) already has precedents of depending on external type definition, one of them was added in the past 6 months to the @types/node types to support the fetch module. This module was originally implemented under undici and later incorporated into Node.js. The source still resides in the undici repo, so to add support for these types they started publishing type definitions for under undici-types that are now a direct depenencey pf the @types/node package (see here).

While it'll be a lot of work, I believe this would bring a lot of value to customers and eventually would help with establishing a single and authoritative source of truth for AWS events that today does not exist. Doing this would benefit not only Powertools TS or the Node.js community but also Powertools Python and the Python community as a whole via Pydantic. Additionally, it would open the door to future use cases like testing factories (#445), integrations with SAM or CDK, and similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussing The issue needs to be discussed, elaborated, or refined internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) parser This item relates to the Parser Utility
Projects
Development

No branches or pull requests

2 participants