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

XMLHttpRequest: Support synchronous requests #338

Closed
kettanaito opened this issue Feb 17, 2023 · 6 comments · Fixed by #619
Closed

XMLHttpRequest: Support synchronous requests #338

kettanaito opened this issue Feb 17, 2023 · 6 comments · Fixed by #619

Comments

@kettanaito
Copy link
Member

Need to explore and verify whether we support synchronous XHR requests. I think we should support them in Node as-is, but not sure how it applies to the browser behavior. There's already a (failing) test committed by a contributor, that's a good starting point.

Since we're using Proxies since #337, I'd expect targeting sync requests shouldn't be a problem.

@kettanaito
Copy link
Member Author

We should be able to support this in Node.js but this won't work in the browser where MSW uses a Service Worker. Synchronous XHR requests do not trigger the "fetch" event of the worker and there's nothing we can do to circumvent that. I wouldn't split a single interception unit (XHR) to cover different behaviors of that request class. That's unreliable and will be extremely hard to maintain, not to mention the amount of architectural changes required to make this happen, in the first place.

@avivasyuta
Copy link
Contributor

Hello. In the current solution, your interceptors break client applications that use synchronous requests.
First, we need to make sure that when we try to make a synchronous request, control is transferred to the original XMLHttpRequest instead of the proxy.

@kettanaito
Copy link
Member Author

@avivasyuta, sounds like a good plan. I think starting with a test for this also makes a good first step. Would you have the chance to submit a pull request so we fix this for everyone?

@avivasyuta
Copy link
Contributor

avivasyuta commented Aug 20, 2023

Sure. I'll try to fix it.

I would also like to ask you to take a look at this PR. It's been hanging for 2 weeks now and you don't seem to be getting notifications.

@kettanaito
Copy link
Member Author

@avivasyuta, I've seen the pull request, I simply didn't have time to get to it yet. I approach open source to the best availability of my time, and sometimes things may take weeks or even months to get a proper review. Since I do this as a hobby in my free time, I hope for your understanding when it comes to timing.

@kettanaito
Copy link
Member Author

Released: v0.34.3 🎉

This has been released in v0.34.3!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/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 a pull request may close this issue.

2 participants