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.

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

refactor(middleware): remove aws-lambda handler #333

Closed
wants to merge 2 commits into from
Closed

refactor(middleware): remove aws-lambda handler #333

wants to merge 2 commits into from

Conversation

baoshan
Copy link
Contributor

@baoshan baoshan commented Aug 31, 2022

This PR fixes #332. Something to be mentioned:

  1. I removed the onUnhandledRequest option. Callers can still custom unhandled requests: use the express * router (as covered by the test cases) or check if the response is a 404.
  2. I removed fromentries because we’re not supporting Node.js v10/12.
  3. I refactored the tests. node-middleware.test.ts and web-worker-handler.test.ts are small now because the source files are small.

I am not sure if someone relies on the aws-lambda middleware. I would like to help (just open an issue).

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 31, 2022
View reviewed changes
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
@baoshan baoshan force-pushed the remove_aws_lambda branch from 1942460 to c754a54 Compare August 31, 2022 08:35
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 31, 2022
View reviewed changes
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
@oscard0m oscard0m requested review from gr2m and wolfy1339 August 31, 2022 08:40
@baoshan baoshan force-pushed the remove_aws_lambda branch from c754a54 to cc6f3d7 Compare August 31, 2022 08:50
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 31, 2022
View reviewed changes
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
test/handle-request.test.ts Fixed Show fixed Hide fixed
@baoshan
Copy link
Contributor Author

baoshan commented Aug 31, 2022

I tried really hard to bypass the “Hard-coded credentials” only found she is smarter than me.

@baoshan baoshan force-pushed the remove_aws_lambda branch from cc6f3d7 to 783de0d Compare August 31, 2022 08:58
@wolfy1339
Copy link
Member

I tried really hard to bypass the “Hard-coded credentials” only found she is smarter than me.

Those can be safely ignored. They are known false positives since in this case they are not valid access tokens and only used in documentation

@wolfy1339
Copy link
Member

Let's split out the changes into a different PR, and focus on removing aws-lamba in this one

@baoshan baoshan force-pushed the remove_aws_lambda branch from 783de0d to 7ddb30b Compare August 31, 2022 17:05
@baoshan
Copy link
Contributor Author

baoshan commented Aug 31, 2022

Great suggestion. This PR only removes the lambda handler.

@oscard0m oscard0m added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Aug 31, 2022
oscard0m
oscard0m reviewed Aug 31, 2022
View reviewed changes
package.json Show resolved Hide resolved
src/index.ts
export { createNodeMiddleware } from "./middleware/node/index";
export {
createCloudflareHandler,
createWebWorkerHandler,
} from "./middleware/web-worker/index";
export { createAWSLambdaAPIGatewayV2Handler } from "./middleware/aws-lambda/api-gateway-v2";
Copy link
Member

Choose a reason for hiding this comment

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

If we are removing exposed methods, should we do an intermediate version deprecating these upcoming removals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this will be released as a major version bump.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this will be released as a major due to the breaking changes. I was just wondering if we want to do an intermediate minor version warning about the upcoming deprecations to our users.

I guess is more a question to the core maintainers.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, that is our usual process. Ideally, a breaking change removes previously deprecated APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it would be great if there was a dedicated npm package that would provide the createAWSLambdaAPIGatewayV2Handler handler. We could release it as an @octokit/* package, but I'd prefer if somone else who actually uses the handler would release it, and we could link to it from the readme and the deprecation message.

Copy link
Member

Choose a reason for hiding this comment

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

I would resolve this conversation for the moment and, if a user comes by with the request we can open a Discussion.

wolfy1339
wolfy1339 approved these changes Aug 31, 2022
View reviewed changes
Copy link
Member

@wolfy1339 wolfy1339 left a comment

Choose a reason for hiding this comment

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

LGTM

@oscard0m oscard0m changed the title refactor(middleware)!: remove aws-lambda handler refactor(middleware): remove aws-lambda handler Aug 31, 2022
@baoshan baoshan marked this pull request as draft September 7, 2022 02:14
@wolfy1339 wolfy1339 mentioned this pull request Sep 7, 2022
Merged
@wolfy1339 wolfy1339 added the Type: Breaking change Used to note any change that requires a major version bump label Nov 7, 2022
@wolfy1339 wolfy1339 changed the base branch from main to beta April 28, 2023 17:23
@wolfy1339 wolfy1339 marked this pull request as ready for review April 28, 2023 17:59
@gr2m gr2m mentioned this pull request May 21, 2023
Merged
@wolfy1339 wolfy1339 deleted the branch octokit:beta February 25, 2024 19:32
@wolfy1339 wolfy1339 closed this Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscard0m oscard0m oscard0m left review comments

@wolfy1339 wolfy1339 wolfy1339 approved these changes

@gr2m gr2m Awaiting requested review from gr2m

Assignees
No one assigned
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove @types/aws-lambda as dependency of this project
4 participants
@baoshan @wolfy1339 @gr2m @oscard0m