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

Refactor webhook handlers #36

Merged
merged 34 commits into from Dec 8, 2022
Merged

Conversation

scopsy
Copy link
Contributor

@scopsy scopsy commented Dec 5, 2022

Summary

Refactored the main POST route contents to use 2 handlers for webhook parsing. The file was really long, and it made testing and maintaining the codebase very hard. This refactor includes:

  • Extracting all the contents of the linear if statement to a new file utils/webhook/linear.handler.ts
  • Extracting all the contents of the github if statement to a new file utils/webhook/github.handler.ts
  • Removed dependency on the req object inside the handlers (body and headers passed as params)
  • Instead of calling res.send inside the handlers, the handlers exit in 2 ways:
    • Throw an API error exception with message and code status
    • Return a status string back to calling function
  • Haven't touched any functionality rather the exist of the handler functions

I hope these changes are the first step to further refactor the handler functions as they contain more duplicated code.

utils/errors.ts Outdated Show resolved Hide resolved
Comment on lines +34 to +38
export class ApiError extends Error {
constructor(public message: string, public statusCode: number) {
super(message);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New exception class used by the handlers, so they can throw instead of calling res.status directly.

import { linearQuery } from "../apollo";
import { ApiError } from "../errors";

export async function githubWebhookHandler(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Contents of this function are the contents of the github if statement previously

Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you replaced early res.send() calls with returns / throws?

pages/api/index.ts Outdated Show resolved Hide resolved
@scopsy
Copy link
Contributor Author

scopsy commented Dec 5, 2022

@tedspare @PeerRich wanted to add some tests with jest but realized it will be close to impossible without refactoring the main function a bit. Would love to hear your thoughts on this one 🙏

});
}
}
} catch (e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this - really clean

@tedspare
Copy link
Collaborator

tedspare commented Dec 5, 2022

Monumental effort @scopsy - this will make the codebase much easier to navigate. Happy to review when ready.

@scopsy
Copy link
Contributor Author

scopsy commented Dec 6, 2022

Will wait until we merge the petitio PR, because will have a lot of conflicts here. After some tests we could merge it aswell 🎉

@scopsy scopsy marked this pull request as ready for review December 6, 2022 16:36
@scopsy
Copy link
Contributor Author

scopsy commented Dec 6, 2022

@tedspare merged conflicts now 🚀

@tedspare tedspare self-requested a review December 6, 2022 16:44
Copy link
Collaborator

@tedspare tedspare left a comment

Choose a reason for hiding this comment

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

LGTM

@tedspare
Copy link
Collaborator

tedspare commented Dec 7, 2022

@scopsy ready to merge?

@scopsy
Copy link
Contributor Author

scopsy commented Dec 8, 2022

Good on my end, we have this deployed internally for the last few days already.

@PeerRich PeerRich merged commit 823e844 into calcom:main Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants