raygun4js
raygun4js copied to clipboard
MindscapeHQ
(Refactor) Minor improvements to NetworkTracking
These changes originally used #433 as the base and were just some things I noticed while chasing down that bug.
- Remove nested promise in the
originalFetch's promisethenfunction.- The resolution of this promise was not handled in the function and was wrapped in an extraneous
try/catch
- The resolution of this promise was not handled in the function and was wrapped in an extraneous
- Moved
disableFetchLoggingto the prototype.- This was implemented but was not a part of the prototype, it appears as though it was meant to be.
- Many methods were being double wrapped in
wrapWithHandler, as the method they were calling was already wrapped.- Reducing the call stack and improving clarity when stepping through the execution of this function.
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 calls executeHandlers which is already wrapped in a try-catch, and Raygun.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 if text().then() throws you get a body returned which says that clone() wasn't available, and the promise is floated.
┌──────────────┐
│ promise.then │
└──────┬───────┘
│
┌───────────────────────────▼──────────────────────────┐
│ │
│ ┌──────────────────┐ │
│ │ if (ourResponse) │ │
│ └────┬─────────┬───┘ │
│ false │ │ │
│ ┌───────┘ └────────┐ │
│ │ │ │
│ │ │ │
│ │ ▼ │
│ │ ┌──────try/catch──────┐ │
│ │ │ │ │
│ │ │ ┌─────────────────┐ │ │
│ ┌────────▼────────┐ │ │ response.text() ├─┼──┼───┐
│ │ executeHandlers │ │ └─────────────────┘ │ │ │
│ └────────┬────────┘ │ │ │ │
│ │ └──────────▲──────────┘ │ │ f
│ ▼ │ │ │ l
│ ┌───────Body─────────┐ │ │ │ o
│ │ N/A when the fetch │ │ │ │ a
│ │ response does not │ This try/catch doesn't │ │ t
│ │ support clone() │ handle anything much. │ │ i
│ └────────────────────┘ a floating promise is │ │ n
│ started here │ │ g
│ │ │
└──────────────────────────────────────────────────────┘ │
│
┌──────────────────────────────┤
│ │
┌────────┴─────────┐ │
│ .then(onResolve) │ │
└────────┬─────────┘ │
│ │
│ │
┌──────────▼──────────┐ │
│ │ │
│ ┌─────────────────┐ │ │
│ │ executeHandlers │ │ │
│ └────────┬────────┘ │ │
│ │with │ │
│ ▼ │ │
│ ┌──────Body──────┐ │ │
│ │ truncated body │ │ │
│ └────────────────┘ │ │
│ │ │
└──────────┬──────────┘ │
│ │
│ │
┌────────▼─────────┐ │
│ .catch(onReject) ◄────────────────────┘
└────────┬─────────┘
│
│
┌───────────▼────────────┐
│ │
│ ┌─────────────────┐ │
│ │ executeHandlers │ │
│ └────────┬────────┘ │
│ │with │
│ ▼ │
│ ┌───────Body─────────┐ │
│ │ N/A when the fetch │ │
│ │ response does not │ │
│ │ support clone() │ │
│ └────────────────────┘ │
│ │
└────────────────────────┘
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):
┌──────────────┐
│ promise.then │
└──────┬───────┘
│
┌───────────────────────────▼─────────────────────┐
│ │
│ ┌───────────────────────────────────────┐ │
│ │ if (ourResponse && ourResponse.text() │ │
│ └──────────────┬─────────┬──────────────┘ │
│ │ │ │
│ ┌───────┘ └────┐ │
│ │ │ │
│ false │ │ true │
│ │ │ │
│ │ │ │
│ │ │ │
│ │ │ │
│ ┌────────▼────────┐ ┌───────▼─────────┐ │
│ │ executeHandlers │ │ response.text() │ │
│ └────────┬────────┘ └───────┬─────────┘ │
│ │ │ │
│ ▼ │ │
│ ┌────────Body────────┐ │ │
│ │ N/A when the fetch │ │ │
│ │ response does not │ │ │
│ │ support clone() │ │ │
│ └──────────┬─────────┘ │ │
│ │ │ │
└─────────────┼──────────────────────┼────────────┘
│ │
▼ │
Promise.resolve │ Return Promise
┌─────────┘
│
┌──────────────────────────────┼─────────────────────────────┐
│ │ │
│ ┌─────────────────┴─────────────┐ │
│ │ │ │
│ │ │ │
│ ┌────────▼─────────┐ ┌────────▼────────┐ │
│ │ .then(onResolve) │ │ .then(onReject) │ │
│ └────────┬─────────┘ └────────┬────────┘ │
│ │ │ │
│ │ │ │
│ ┌──────────▼──────────┐ ┌───────────▼─────────────┐ │
│ │ │ │ │ │
│ │ ┌─────────────────┐ │ │ ┌─────────────────┐ │ │
│ │ │ executeHandlers │ │ │ │ executeHandlers │ │ │
│ │ └────────┬────────┘ │ │ └────────┬────────┘ │ │
│ │ │with │ │ │with │ │
│ │ ▼ │ │ ▼ │ │
│ │ ┌──────Body──────┐ │ │ ┌───────Body──────────┐ │ │
│ │ │ truncated body │ │ │ │ N/A response.text() │ │ │
│ │ └────────────────┘ │ │ │ rejected: reason │ │ │
│ │ │ │ └─────────────────────┘ │ │
│ └─────────────────────┘ │ │ │
│ └─────────────────────────┘ │
│ │
└──────────────────────────────┬─────────────────────────────┘
│
▼
Handled by parent promise catch
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 👌
Hey @Codex-, we must have missed this one. I have created a ticket to action this, so we should have some feedback coming your way once we get to this. Thanks for your work here :)
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 🙂
