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

(Refactor) Minor improvements to NetworkTracking #434

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Codex-
Copy link
Contributor

@Codex- Codex- commented Nov 25, 2021

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 promise then function.
    • The resolution of this promise was not handled in the function and was wrapped in an extraneous try/catch
  • Moved disableFetchLogging to 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

@Codex- Codex- force-pushed the feature/fetch_handling_improvements branch from e40153e to 7aa0eec Compare November 26, 2021 03:29
@Codex-
Copy link
Contributor Author

Codex- commented Nov 26, 2021

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-
Copy link
Contributor Author

Codex- commented Mar 18, 2022

Would be great to get some feedback on this :)

@Callum-McKay
Copy link
Contributor

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

@Widdershin Widdershin self-requested a review May 25, 2022 05:05
@Codex- Codex- force-pushed the feature/fetch_handling_improvements branch from e88a5e1 to 9c0dd96 Compare July 21, 2022 23:05
@Codex-
Copy link
Contributor Author

Codex- commented Jul 21, 2022

Rebased on tip of master.

- 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.
@Codex- Codex- force-pushed the feature/fetch_handling_improvements branch from 9c0dd96 to 1fed759 Compare September 20, 2023 22:57
@Codex-
Copy link
Contributor Author

Codex- commented Sep 20, 2023

Rebased 👀

@Hamish-taylor
Copy link
Contributor

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 🙂

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