refactor: #197 Refactor to use Promises internally #200
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Sumitra Manga <[email protected]>
miquelbeltran
commented
May 8, 2024
Comment on lines
+89
to
+91
| const errorMessage = `Error is too large to send to Raygun (${messageSize}kb)\nStart of error: ${startOfMessage}`; | ||
| console.error(`[Raygun4Node] ${errorMessage}`); | ||
| throw Error(errorMessage); |
There was a problem hiding this comment.
Throw error upwards if the reported error message is too big, otherwise users will not notice.
See issue #140
miquelbeltran
commented
May 8, 2024
Comment on lines
+244
to
+257
| .then((response) => { | ||
| const durationInMs = stopTimer(); | ||
| debug( | ||
| `[raygun.ts] Successfully sent message (duration=${durationInMs}ms)`, | ||
| ); | ||
| return response; | ||
| }) | ||
| .catch((error) => { | ||
| const durationInMs = stopTimer(); | ||
| debug( | ||
| `[raygun.ts] Error sending message (duration=${durationInMs}ms): ${error}`, | ||
| ); | ||
| return error; | ||
| }); |
There was a problem hiding this comment.
Wrap the transport.send call with a then/catch so we can log our own error/success messages, but still send the results back to the end user.
This is what was done in wrappedCallback.
miquelbeltran
commented
May 8, 2024
Comment on lines
+264
to
+282
| sendWithCallback( | ||
| exception: Error | string, | ||
| customData?: CustomData, | ||
| callback?: Callback<IncomingMessage>, | ||
| request?: RequestParams, | ||
| tags?: Tag[], | ||
| ) { | ||
| // call async send but redirect response to provided legacy callback | ||
| this.send(exception, customData, request, tags) | ||
| .then((response) => { | ||
| if (callback) { | ||
| callVariadicCallback(callback, null, response); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| if (callback) { | ||
| callVariadicCallback(callback, error, null); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Legacy send with a callback, wraps the new async send call.
IMO should be removed in the future.
miquelbeltran
commented
May 8, 2024
Comment on lines
+162
to
+179
| test("send with expressHandler custom data", function (t) { | ||
| t.plan(1); | ||
| let client = new Raygun.Client().init({ | ||
| apiKey: API_KEY, | ||
| }); | ||
|
|
||
| client.expressCustomData = function () { | ||
| return { test: "data" }; | ||
| }; | ||
| client._send = client.send; | ||
| client.send = (err, data, params, tags) => { | ||
| client.send = client._send; | ||
| t.equal(data.test, "data"); | ||
| t.end(); | ||
| return Promise.resolve(null); | ||
| }; | ||
| client.expressHandler(new Error(), {}, {}, function () {}); | ||
| }); |
There was a problem hiding this comment.
Test is moved to raygun_async_send because express handler now uses async send internally.
miquelbeltran
commented
May 8, 2024
test/raygun_send_test.js
Outdated
There was a problem hiding this comment.
This test file should be removed when we remove sendWithCallback as well.
miquelbeltran
changed the title
miquelbeltran
requested review from
a team,
TheRealAgentK,
PanosNB and
sumitramanga
and removed request for
a team
sumitramanga
approved these changes
May 9, 2024
|
thanks @sumitramanga ! I will wait to merge until |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refactor: #197 Refactor to use Promises internally
Description 📝
raygun.transport.tsto use Promises instead of callbaks #197raygun.transport.tsshould be ignored #198Type of change
Updates
SendOptionsinternal type.raygun.transport.tsto return a Promise instead of using a callback.raygun.sync.worker.tsto the changes inraygun.transport.ts.raygun.batch.tsto the changes inraygun.transport.ts.send()method to implement logic directly instead of depending on legacysendWithCallback.sendWithCallback()to use newsend()method internally.Test plan 🧪
Author to check 👓
Reviewer to check ✔️