-
Notifications
You must be signed in to change notification settings - Fork 60
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) Minor improvements to NetworkTracking #434
base: master
Are you sure you want to change the base?
(Refactor) Minor improvements to NetworkTracking #434
Conversation
e40153e
to
7aa0eec
Compare
Updated the first comment with some diagrams and docs explaining the changes I've made, let me know if you spot any issues or have questions 👌 |
Would be great to get some feedback on this :) |
Hey @Codex-, we must have missed this one. |
e88a5e1
to
9c0dd96
Compare
Rebased on tip of |
- Returning this promise allows the parent catch block to handle the promise rejection.
- The promise started handles its own exceptions. - The call to self.executeHandlers is already wrapped with a try catch.
- executeHandlers has already been wrapped with wrapWithHandler.
- Allows `disableFetchLogging` to function.
- This gives the body being passed to the handlers more meaning.
9c0dd96
to
1fed759
Compare
Rebased 👀 |
Hey @Codex- we really appreciate your continued contributions to Raygun4js! We have had a look over your PR and everything looks good at a glance. I will kick of an internal code review to dig a little deeper. I am hoping to get this included in the next release 🙂 |
These changes originally used #433 as the base and were just some things I noticed while chasing down that bug.
originalFetch
's promisethen
function.try/catch
disableFetchLogging
to the prototype.wrapWithHandler
, as the method they were calling was already wrapped.In addition to this, I have completed some reworking on how
response.text()
is handled, as the rejection can be misleading when the body is passed to the response handlers.executeResponseHandlers
internally callsexecuteHandlers
which is already wrapped in a try-catch, andRaygun.Utilities.truncate
has appropriate checks in place to prevent throwing.The usage of
.text()
also floats a promise which while it gets caught in its own catch does not call to the error handling catch provided for the parent promise.Old flow:
Here you can see that if
response.text()
throws you get an unusual error that states the response doesn't support.clone()
where at this point we know it does. The flow shows that if either.text()
throws or iftext().then()
throws you get a body returned which says thatclone()
wasn't available, and the promise is floated.This is my proposed new flow, it either resolves with the original body should the response not be able to be cloned, or returns the promise and handles the text resolution or rejection (and since the promise is returned the parent promise catch handles both should they throw):