8000 fix: specify event type explicitly by contexD · Pull Request #1295 · middyjs/middy · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: specify event type explicitly #1295

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.

8000
Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 27, 2025
Merged

fix: specify event type explicitly #1295

merged 2 commits into from
Apr 27, 2025

Conversation

contexD
Copy link
Contributor
@contexD contexD commented Apr 23, 2025

  • fixes the issue of the event type not being correctly inferred by the Typescript compiler since the infer keyword does not properly work with generic types

  • change adopts the same solution as for middyjs#1228: Handler return is not type-checked. , that is adding TEvent as a type parameter to the MiddlewareHandler allows to explicitly indicate the handler's event type

closes #1293

@contexD contexD requested a review from a team as a code owner April 23, 2025 12:37
@contexD contexD force-pushed the fix/infer-event-type branch from 3e0d230 to 190bd4d Compare April 23, 2025 14:44
willfarrell
willfarrell previously approved these changes Apr 23, 2025
View reviewed changes
@contexD
fix: specify event type explicitly
c6a3eca 
- fixes the issue of the event type not being correctly inferred by the
  Typescript compiler since the `infer` keyword does not properly work
  with generic types

- change adopts the same solution as for [middyjs#1228: Handler return
  is not type-checked.](middyjs#1228) ,
  that is adding `TEvent` as a type parameter to the `Middlewarehandler`
  allows to explicitly indicate the handler's event
  type

closes[middyjs#1293](middyjs#1293)

Signed-off-by: contexD <39738016+contexD@users.noreply.github.com>
@contexD contexD force-pushed the fix/infer-event-type branch from 190bd4d to c6a3eca Compare April 24, 2025 09:06
@contexD contexD requested a review from willfarrell April 25, 2025 07:13
@willfarrell willfarrell changed the base branch from main to develop April 27, 2025 15:11
@willfarrell willfarrell merged commit fe06aab into middyjs:develop Apr 27, 2025
13 of 14 checks passed
@willfarrell
Copy link
Member

Thanks for the PR!

@sergiojoker11
Copy link
sergiojoker11 commented May 16, 2025 •
edited
Loading

thanks for the PR! Nonetheless, this is a breaking change as per SemVer. I know it is too late cause it has been released and all, but some migration notes can be provided to make lib users easier.

For us we had to change from:

export function unprotectedEndpointMiddleware<TConfig, T extends z.AnyZodObject>(
  configReaderFn: ConfigReaderFn<Promise<TConfig>>,
  eventSchema: T,
  enableCors: boolean
): middy.MiddyfiedHandler<APIGatewayProxyEvent, APIGatewayProxyResult> {
  return baseConfigMiddleware(() => configReaderFn(), true)
    <Omitted some more Middys>
    .use(enableCors ? corsMiddleware() : noOpMiddleware) as middy.MiddyfiedHandler<
    APIGatewayProxyEvent,
    APIGatewayProxyResult
  >;
}

to

export function unprotectedEndpointMiddleware<TConfig, T extends z.AnyZodObject>(
  configReaderFn: ConfigReaderFn<Promise<TConfig>>,
  eventSchema: T,
  enableCors: boolean
): middy.MiddyfiedHandler {
  return baseConfigMiddleware(() => configReaderFn(), true)
    <Omitted some more Middys>
    .use(enableCors ? corsMiddleware() : noOpMiddleware)
}

No how the use of "as" dissappeared/casting.

the issue appeared on usages of above middy:

unprotectedEndpointMiddleware(<ommited>).handler<Event>(myHandlerFunction)

transpiling issue:

src/lambdas/api/post-refresh/index.ts:77:18 - error TS2345: Argument of type '(event: { path: string; lambdaConfig: BaseRefreshConfig & CorsEndpointConfig & UserBaseStoreEndpointConfig; ... 14 more ...; stageVariables?: Record<...> | ... 1 more ... | undefined; }, ctx: Context) => Promise<...>' is not assignable to parameter of type 'never'.

77 ).handler<Event>(lambdaHandler);
                    ~~~~~~~~~~~~~

most likely this is the detonator: https://github.com/middyjs/middy/pull/1295/files#r2092966279

SemVer FAQ

sergiojoker11
sergiojoker11 reviewed May 16, 2025
View reviewed changes
packages/core/index.d.ts
TResult = any,
TEvent = any
> =
THandler extends LambdaHandler<TEvent, TResult> // always true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anchor

@willfarrell
Copy link
Member

A PR to further improve types is always welcome.

@sergiojoker11
Copy link

A PR to further improve types is always welcome.

oh yeah, definitely. Just adding this comment to help others and asking you guys to help further by flagging this as a hidden breaking change in the release notes in order to help others even further.

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

@sergiojoker11 sergiojoker11 sergiojoker11 left review comments

@gordonagnew gordonagnew gordonagnew approved these changes

@jonathanstumm jonathanstumm jonathanstumm approved these changes

@willfarrell willfarrell Awaiting requested review from willfarrell

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handler event type is not correctly inferred when using type parameters
5 participants
@contexD @willfarrell @sergiojoker11 @gordonagnew @jonathanstumm
0