style(event-handler): apply stricter linting #4578

sdangol · 2025-09-25T18:09:58Z

Summary

This PR resolves all the linting issues resulting from the new rules being introduced in #4545

Changes

Please provide a summary of what's being changed

Removed async from functions not doing await

Please add the issue number below, if no issue is present the PR might get blocked and not be reviewed

Issue number: closes #4575

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

sdangol · 2025-09-25T18:13:52Z

@svozza There were some any being used in this file. There were some tests which was checking for handling null values. I wanted to check with you if those tests are needed if the null values aren't allowed for those parameters like headers, multiValueHeaders?

dreamorosi · 2025-09-25T18:45:59Z

packages/event-handler/src/appsync-graphql/AppSyncGraphQLResolver.ts


    return this.#withErrorHandling(
-      () => this.#executeSingleResolver(event, context, options),
+      async () => await this.#executeSingleResolver(event, context, options),


Can we just pass the unresolved promise here?

Yeah, because as this is with this change, it slightly changes the behaviour from what it was.

dreamorosi · 2025-09-25T18:47:18Z

packages/event-handler/src/types/rest.ts

 type RouteHandler<TReturn = HandlerResponse> = (
  reqCtx: RequestContext
-) => Promise<TReturn>;
+) => Promise<TReturn> | TReturn;


svozza · 2025-09-25T20:31:08Z

packages/event-handler/src/appsync-graphql/AppSyncGraphQLResolver.ts


    return this.#withErrorHandling(
-      () => this.#executeSingleResolver(event, context, options),
+      async () => await this.#executeSingleResolver(event, context, options),


Yeah, because as this is with this change, it slightly changes the behaviour from what it was.

svozza · 2025-09-25T20:31:58Z

packages/event-handler/src/appsync-graphql/AppSyncGraphQLResolver.ts

    context: Context,
    options?: ResolveOptions
-  ): Promise<unknown> {
+  ) {


This function can still return a promise, the type signature should reflect that.

There are quite a few uses of unknown in the AppSyncGraphQLResolver. Do you have some suggestions on how we could we narrow down the return types for this?

svozza · 2025-09-25T20:42:57Z

packages/event-handler/src/types/rest.ts

 };

-type NextFunction = () => Promise<HandlerResponse | void>;
+type NextFunction = () =>


I'm worried this change is going to encourage people not to await the next function because they think it being async is optional. Failing to await next causes the an error to be thrown and the whole requst fails.

I got an idea (haven't tested it tho):

What if whenever someone calls app.use we wrap their middleware arrow function in a function we control, before putting the wrapped middleware in the array.

Our function becomes the next and doesn't care if it's called with await or not, but it will always await the function it's wrapping before returning its result.

This way (assuming I'm not just saying something dumb), it doesn't matter if customers forget to call await next or even if the middleware is asynchronous or synchronous, we'll always treat it as async.

So the issue is not with awaiting the middleware, it's the step after that, which is actually happening inside the the middlware handler. Not calling await returns immediately and messes up the execution order. Even when I tested capturing the Promise and forcing an await inside the composeMiddleware function, the problem was still there.

ok then - @sdangol I think this discussion is relevant to your question below.

dreamorosi · 2025-09-26T08:22:18Z

Hi @sdangol - can you please address the feedback and get this merged in the next couple hrs? Thank you.

svozza · 2025-09-26T09:00:30Z

@svozza There were some any being used in this file. There were some tests which was checking for handling null values. I wanted to check with you if those tests are needed if the null values aren't allowed for those parameters like headers, multiValueHeaders?

Ah yeah, that test is a hangover from a bug where we used to accept null as a value with headers but it can only ever be undefined (see the type definition) so you can get rid of it.

…constructor any, added unknown to the return type of some functions in GraphQL resolver

sdangol · 2025-09-26T09:35:15Z

I'm struggling a bit to fix this one build issue in the cors middleware where we exit early without returning anything.

sdangol · 2025-09-26T09:36:10Z

packages/event-handler/src/types/rest.ts

 ) => Promise<HandlerResponse>;

 interface ErrorConstructor<T extends Error = Error> {
+  // biome-ignore lint/suspicious/noExplicitAny: this is a generic type that is intentionally open


@svozza @dreamorosi Can we narrow this down to something? I wasn't sure if we could

I guess the issue here is supporting custom error types, which can have arbitrary parameters when constructed:

class CustomError extends Error { constructor(/* these parameters can be literally anything */) { } }

powertools-for-aws-oss-automation · 2025-09-26T10:38:18Z

Not all issues are linked correctly.

Please link each issue to the PR either manually or using a closing keyword in the format fixes #<issue-number> format.

If mentioning more than one issue, separate them with commas: i.e. fixes #<issue-number-1>, closes #<issue-number-2>.

sdangol · 2025-09-26T10:41:45Z

@svozza @dreamorosi For the use of void in unions, I wasn't able to find a way through it so I added an ignore for the lint rule. Narrowing the unknowns on the AppSyncResolver might need more work and might not be suitable to block this PR. Rest of the issues, I've addressed them

sonarqubecloud · 2025-09-26T10:49:11Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
4.3% Duplication on New Code

See analysis details on SonarQube Cloud

sdangol added 3 commits September 25, 2025 18:28

Fixed lint errors

bc527d3

Removeed some use of any

3b89362

Added options param

7311816

boring-cyborg bot added event-handler This item relates to the Event Handler Utility tests PRs that add or change tests labels Sep 25, 2025

sdangol self-assigned this Sep 25, 2025

pull-request-size bot added the size/L PRs between 100-499 LOC label Sep 25, 2025

dreamorosi reviewed Sep 25, 2025

View reviewed changes

svozza reviewed Sep 25, 2025

View reviewed changes

sdangol added 2 commits September 26, 2025 10:06

removed tests which were testing for null headers, ignored the error …

e3759c5

…constructor any, added unknown to the return type of some functions in GraphQL resolver

fixed test coverage

2c2ef47

sdangol commented Sep 26, 2025

View reviewed changes

sdangol added 2 commits September 26, 2025 11:10

reverted changes in the nextfunction type

6ae3368

fixed lint issue

167c4ec

sdangol marked this pull request as ready for review September 26, 2025 10:35

sdangol changed the title ~~style(event-handler): apply stricter linting~~ style(event-handler): applied stricter linting Sep 26, 2025

sdangol changed the title ~~style(event-handler): applied stricter linting~~ style(event-handler): apply stricter linting Sep 26, 2025

reverted changes in the compose middleware function

bf576e2

dreamorosi self-requested a review September 26, 2025 10:48

dreamorosi approved these changes Sep 26, 2025

View reviewed changes

dreamorosi merged commit 3566aa2 into ci/fix_linting Sep 26, 2025
32 of 34 checks passed

dreamorosi deleted the style/event-handler branch September 26, 2025 10:49

dreamorosi linked an issue Sep 26, 2025 that may be closed by this pull request

Maintenance: fix linting issues for the event-handler package #4575

Closed

2 tasks

style(event-handler): apply stricter linting #4578

style(event-handler): apply stricter linting #4578

Uh oh!

Conversation

sdangol commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Uh oh!

sdangol commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamorosi Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svozza Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamorosi commented Sep 26, 2025

Uh oh!

svozza commented Sep 26, 2025

Uh oh!

sdangol commented Sep 26, 2025

Uh oh!

sdangol Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powertools-for-aws-oss-automation bot commented Sep 26, 2025

Uh oh!

sdangol commented Sep 26, 2025

Uh oh!

sonarqubecloud bot commented Sep 26, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sdangol commented Sep 25, 2025 •

edited

Loading

sdangol commented Sep 25, 2025 •

edited

Loading

dreamorosi Sep 25, 2025 •

edited

Loading

svozza Sep 26, 2025 •

edited

Loading

sdangol Sep 26, 2025 •

edited

Loading