Open swanson opened 3 years ago
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.
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?
Resolves #489
Per guidance here: https://developers.google.com/web/updates/2017/09/abortable-fetch#reacting_to_an_aborted_fetch
Users may choose how to handle the rejected promise in their own code, but the profiler will not raise an uncaught exception.