-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
fix(xhr): invoke "onloadend" even if response was not returned from the interceptor #387
Conversation
Closing this, because I did some more digging. I’ll open an issue instead to discuss how a proper fix could look like. |
Reopening this with an updated description and fix, because I found the root cause of the issue and a proper fix for it. |
Thanks for looking into this, @hpohlmeyer! To double-check, have you encountered this issue while working with the latest version of this library? Since it's not the version most people install when installing |
xhr.addEventListener('error', resolveDelayed) | ||
xhr.addEventListener('load', resolveDelayed) | ||
}), | ||
xhr.send(null), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since xhr.send
doesn't return a Promise, is there a reason to put these two in a single Promise.all
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to avoid adding the event listeners after the send
call.
xhr.send(null)
await new Promise((resolve) => {
const resolveDelayed = () => setTimeout(resolve, 1000)
xhr.addEventListener('error', resolveDelayed)
xhr.addEventListener('load', resolveDelayed)
})
seems to work fine though. Do think that’s fine or would you rather see a more specific version like:
xhr.send(null)
const finishedPromise = new Promise((resolve) => {
const resolveDelayed = () => setTimeout(resolve, 1000)
xhr.addEventListener('error', resolveDelayed)
xhr.addEventListener('load', resolveDelayed)
})
xhr.send(null)
await finishedPromise
test/modules/XMLHttpRequest/compliance/xhr-event-handlers.browser.test.ts
Outdated
Show resolved
Hide resolved
Yes, I am using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fantastic change! Thank you, @hpohlmeyer. Looks ready to me!
Released: v0.22.16 🎉This has been released in v0.22.16! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
The
onloadend
event handler is not being called if we do not send mocked response from a request interceptor:This is caused by
onloadend
not being located onXMLHttpRequest
, but onXMLHttpRequestEventTarget
which is part of its prototype chain (XMLHttpRequest
<-XMLHttpRequestPrototype
<-XMLHttpRequestEventTargetPrototype
).Because of that the property descriptor for
onloadend
is not found and we patch it on theXMLHttpRequest
. When the browser calls the setter foronloadend
, it does that onXMLHttpRequestEventTarget
, which is why our proxy does not get notified.This PR fixes the issue by walking the prototype chain, until it finds the property it is looking for. Redefining the property or looking for a property descriptor is done on the same level where the property is defined.
The vitest tests in
xhr-event-handlers.test.ts
do not complain about the issue, I am not sure in which environment they are running, but it is an issue in browsers. I have added a playwright test because of that.As a sidenote: While looking for a fix, I found https://github.com/mswjs/interceptors/blob/v0.22.15/src/interceptors/XMLHttpRequest/XMLHttpRequestController.ts#L51-L64, which looks like it could be caused by a similar issue. I have removed the highlighted part and ran the tests. They run without errors, so if there is a test covering that part it might be fixed.