I'm going to test this out tomorrow in my app since it doesn't seem like there are any automated tests for the Javascript code.

Maybe we re-throw instead of logging for e.name !== 'AbortError' ?

I am still somewhat confused at how calling a non patched "fetch" works given that the promise would be aborted.

This is my understanding of the situation, but I am also new to using AbortController!

In my application code, I have something like this:

export default class extends Controller {
  connect() {
    this.controller = new AbortController();
  }

  show() {
    const { signal } = this.controller;

    fetch("/some/endpoint", { signal })
      .then(r => r.text())
      .then(html => {
        // do some stuff
        console.log(html);
      })
      .catch(() => {});
  }
  
  hide() {
    this.controller.abort();
  }
}

If we hide before the AJAX request is finished, we want to abort the pending request. The fetch promise is rejected and we catch it and suppress any errors.

But the profiler has patched fetch (here) and added on an additional then chain. So it's the equivalent of

fetch("/some/endpoint", { signal })
    .then(r => r.text())
    .then(html => {
      // do some stuff
      console.log(html);
    })
    .catch(() => {})
    .then(response => {
      // do the rack-mini-profiler stuff since the response is finished
    })

Since the additional then from rack-mini-profiler is after the catch block, the error gets thrown.

Codecov Report

Merging #490 (2ddd6d4) into master (94670b7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #490   +/-   ##
=======================================
  Coverage   88.64%   88.64%           
=======================================
  Files          18       18           
  Lines        1250     1250           
=======================================
  Hits         1108     1108           
  Misses        142      142           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94670b7...2ddd6d4. Read the comment docs.

This is where I am getting really confused:

From my reading of this code it should be running:

fetch("/some/endpoint", { signal })
    .then(response => {
      // do the rack-mini-profiler stuff since the response is finished
    })
    .then(r => r.text())
    .then(html => {
      // do some stuff
      console.log(html);
    })
    .catch(() => {})
   

Something about the implementation in mini profiler does not feel right, maybe we should refactor this code a bit so it is clearer, this giant try catch nest is not that easy to understand.

Ah, I think you are right and maybe this https://github.com/MiniProfiler/rack-mini-profiler/blob/master/lib/html/includes.js#L983-L985 is actually where the AbortError is getting logged in my application. I will investigate more and see if I can narrow it down.

edit: I think I am now equally confused! If the request is aborted then it should not even get into the patched fetch at all! Hmm.

After more testing, I think we were both wrong. The Promise chain "forks" since RMP returns the original fetch chain, not the one that the additional "look for x-mini-profile-ids" then is attached to.

This makes sense because even when I was getting the unhandled exceptions, my app was still working correctly.

fetch  -> rack-mini-profiler chain (no exception handling)
      \
        -> application code chain (user controlled exception handling)

So whatever my application code does has no bearing on the exception handling in the profiler (and vice-versa). I think the original solution then would be best: the profiler will need to shallow the AbortError to avoid to extra noise in JS error tracker and confusion when developing.

The PR as it currently exists seems good to me and resolves my issue after testing.

Just lost 3 hours debugging this until I found that the mini-profiler monkey-patches window.fetch. Any chance of getting this merged?

We also spent way too much time trying to figure out why this error was appearing in our error tracker. Any chance of getting this merged?

I just stumbled onto this as well, seeing "Uncaught (in promise)" in my browser logs when I have a catch in my application code, though unrelated to aborting requests.

What @swanson says about the promise chain "forking" seems to make sense to explain why I'm seeing errors, with the stacktrace pointing back to RMP.

But I wonder, should RMP only swallow the AbortError, as this PR currently is, or should it swallow everything, as was proposed in the original issue report? If the chains are truly forked and the application's chain is separate from RMP's chain (as opposed to appending to one another), then I think RMP should be swallowing everything, or the same issue could be raised later for any other reason fetch might reject the promise.

For example, running this code (assuming you have nothing listening/responding locally on port 1234, simulating a non-abort error), trying to demonstrate the separate chains:

const f = fetch('http://127.0.0.1:1234');

// RMP
f.then(() => {});

// Application
f.catch(() => console.log('caught from application'));

You'll see you get the "caught from application" log line, as handled by the application, but you'll also get the "Uncaught (in promise)" error, as if both chains are acting independently.

If you change the RMP line to

f.then(() => {}).catch(() => {});

You'll still get the "caught from application" log line, so that side didn't break, but you no longer get the "Uncaught (in promise)" error on the RMP side.

Best I can tell, the order doesn't matter, so whether the RMP chain is set up first, or the application chain, the result is the same.

Just for kicks, if you remove the RMP and application chains and replace it with a single chain (simulating @SamSaffron's assumption earlier of appending rather than forking):

f.then(() => {}).then(() => {}).catch(() => console.log('caught from application'));

You'll get the "caught from application" line, and no "Uncaught (in promise)" line, but since RMP returns the result of fetch itself, not the chain with the then, I don't think this accurately describes the current state of the code.

I only tested this in Chrome (v131.0.6778.205), but hopefully this behavior is true across browsers. I'm also far from an expert on promises, so take this all with a grain of salt.

Alternatively, maybe RMP should be returning the chain after then, rather than the original fetch, which would likely avoid the forking in the first place, instead treating it truly like appending, but then I think we'd have to be mindful that RMP's then needs to always return the original response argument so the application-side then has access to the same object. But then if RMP's then threw an error, we'd probably need to ensure that didn't interfere with running the application's then's as an RMP error likely doesn't imply an application error. If the chains are truly forked, simply swallowing everything on the RMP side might be safest, letting the application have it's own, clean chain.