MindscapeHQ / raygun4js

JavaScript provider for Raygun
https://raygun.com
Other
149 stars 60 forks source link

(Refactor) Minor improvements to NetworkTracking #434

Open Codex- opened 3 years ago

Codex- commented 3 years ago

These changes originally used #433 as the base and were just some things I noticed while chasing down that bug.

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
Codex- commented 3 years ago

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 👌

Codex- commented 2 years ago

Would be great to get some feedback on this :)

Callum-McKay commented 2 years ago

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 :)

Codex- commented 2 years ago

Rebased on tip of master.

Codex- commented 1 year ago

Rebased 👀

Hamish-taylor commented 1 year ago

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 🙂