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

feat: #146 async send support #181

Merged
merged 23 commits into from
May 7, 2024
Merged

feat: #146 async send support #181

merged 23 commits into from
May 7, 2024

Conversation

miquelbeltran
Copy link
Contributor

@miquelbeltran miquelbeltran commented May 2, 2024

feat: #146 async send support

Description 📝

Note: the client requires a major clean-up to fully remove all callbacks from the internal implementation. This will be done in separated PRs after this one.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Updates

  • The original send with a callback was renamed to sendWithCallback, it is still being used internally, so it was hard to fully remove the existing callback parameter without doing a major package rewrite.
  • Created a new send method that supports awaited calls.
    • New method returns a Promise that can be an IncomingMessage. Users can ignore this response if they want, or used to verify that the error was sent correctly to Raygun.
    • If the method fails, the Promise will be "rejected", users can catch this error. The error payload can be either Error & IncomingMessage or just Error.
    • To implement this async method, I used the Promise constructor, wrapping the legacy sendWithCallback method.

Other changes:

  • Reviewed the internal send implementations, they still use callbacks internally, and will require a lot of work to clean up. This work should be done in small batches split in several PRs.
  • Updated README.md with usage examples.
  • Updated example examples/using-domains.
    • note that the express example doesn't use the send() method directly.

Test plan 🧪

  • Added a new unit test suite that copies the original raygun_send_test but for async send calls.
  • Updated and tested on the examples/using-domains, which now uses the async send method.

Author to check 👓

  • Project and all contained modules builds successfully
  • Self-/dev-tested
  • Unit/UI/Automation/Integration tests provided where applicable
  • Code is written to standards
  • Appropriate documentation written (code comments, internal docs)

Reviewer to check ✔️

  • Project and all contained modules builds successfully
  • Change has been dev-/reviewer-tested, where possible
  • Unit/UI/Automation/Integration tests provided where applicable
  • Code is written to standards
  • Appropriate documentation written (code comments, internal docs)

Comment on lines -250 to -260
customData?: CustomData,
callback?: (err: Error | null) => void,
request?: RequestParams,
tags?: Tag[],
): void {
const result = this.buildSendOptions(
exception,
customData,
callback,
request,
tags,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all the unused parameters, although this would be out of scope for this PR

Comment on lines +321 to +325
this.send(err, customData || {}, requestParams, [
"UnhandledException",
]);
]).catch((err) => {
console.log(`[Raygun] Failed to send Express error: ${err}`);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is internal to the express error handler.

Comment on lines -20 to -31
const send = (e) =>
new Promise((resolve, reject) => {
raygunClient.send(e, null, (err, data) => {
if (err) {
reject(err);
} else {
resolve(data);
}
});
});

await send(new Error("offline error"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is no longer necessary, because now the raygunClient.send method is asynchronous

@@ -27,7 +27,7 @@ test("send basic", {}, function (t) {
var client = new Raygun.Client().init({
apiKey: API_KEY,
});
client.send(new Error(), {}, function (response) {
client.sendWithCallback(new Error(), {}, function (response) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old raygun_send_test still uses the legacy sendWithCallback method, this test fine can be removed when the sendWithCallback method is finally removed.

@miquelbeltran miquelbeltran changed the title feat: #146 async send support (WIP) feat: #146 async send support May 6, 2024
@miquelbeltran miquelbeltran marked this pull request as ready for review May 6, 2024 09:46
@miquelbeltran miquelbeltran requested review from a team, TheRealAgentK and PanosNB and removed request for a team May 6, 2024 09:46
@miquelbeltran miquelbeltran requested a review from sumitramanga May 6, 2024 09:46
lib/raygun.ts Outdated Show resolved Hide resolved
sumitramanga
sumitramanga previously approved these changes May 6, 2024
Copy link
Collaborator

@sumitramanga sumitramanga left a comment

Choose a reason for hiding this comment

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

Implementation overall LGTM! Nice work on downsizing the send call 💪

Co-authored-by: Sumitra Manga <[email protected]>
@miquelbeltran
Copy link
Contributor Author

I need a new review... 🙃

The tickets have been created, I'll edit them with some extra info.

@paul-uz
Copy link

paul-uz commented May 7, 2024

What's the ETA on getting this merged and released?

I'm super keen to try it out and get it hooked into Winston as a custom transport.

@miquelbeltran
Copy link
Contributor Author

Hi @paul-uz we are talking internally that we want to do a pre-release with these changes once they are merged in. I'll try to remember to ping you when we have it ready, and be looking forward to your feedback!

@paul-uz
Copy link

paul-uz commented May 7, 2024

Please do what you can to extradite this ✌️

@sumitramanga
Copy link
Collaborator

Just double checking - Did you turn all to do's into an Issue?

Copy link
Collaborator

@sumitramanga sumitramanga left a comment

Choose a reason for hiding this comment

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

Thank you for the changes and making those Issues! LGTM🍾

@miquelbeltran
Copy link
Contributor Author

Just double checking - Did you turn all to do's into an Issue?

yes, I did!

@miquelbeltran miquelbeltran merged commit 6441240 into develop May 7, 2024
5 checks passed
@miquelbeltran miquelbeltran deleted the 146-async-await branch May 7, 2024 21:17
@paul-uz
Copy link

paul-uz commented May 7, 2024

@miquelbeltran how long until a new release version is out?

@sumitramanga sumitramanga mentioned this pull request May 7, 2024
11 tasks
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.

3 participants