Breaks XMLHttpRequest sync mode logic in browser environment

[esjs]

Describe the bug

I've noticed a problem with XMLHttpRequest when it set up to work in synchronous mode

Environment

• msw: 0.28.0
• nodejs: 12.16.3
• npm: 6.14.4

Google Chrome Version 89.0.4389.90 (Official Build) (x86_64)

To Reproduce

Steps to reproduce the behavior:

1. Download attached demo project demo.zip
2. Run yarn && yarn start to start project
3. See browser console. There are two logs for SYNC XMLHttpRequest result and for onload event

Expected behavior

SYNC result should be available, but only onload result is present (see first and second screenshots). Also, MSW didn't catch this request (but it's not critical in my use case). And yes, I know, that synchronous requests are bad, but we use jdPDF which synchronously loads dynamically added fonts, and when combined with MSW mocking this leads to errors.

Screenshots

But, if we disable worker.start everything starts to work as expected

I hope there is an easy fix for this or I'm missing something.

Thank you, Vladimir

[kettanaito]

Hey, @esjs. Thanks for reporting this.

We are using @mswjs/interceptors library that provides the interception of requests in Node.js. That includes handling XMLHttpRequests where the issue surfaces. We currently have no tests for the sync XMLHttpRequest and that's a great opportunity to add them.

Please, would you be interested in sending a pull request with a failing test to that repository? It can be put in the test/interept/XMLHttpRequest directory.

[esjs]

HI @kettanaito, sure, no problem.

I've just tried and I can't make test to run. Can this repo run tests on its own or only as part of msw project?

I get an error when I try to run yarn test:integration:browser "ModuleNotFoundError: Module not found: Error: Can't resolve 'node-request-interceptor' in '/Users/username/interceptors/test/intercept/fetch'

[msutkowski]

HI @kettanaito, sure, no problem.

I've just tried and I can't make test to run. Can this repo run tests on its own or only as part of msw project?

I get an error when I try to run yarn test:integration:browser "ModuleNotFoundError: Module not found: Error: Can't resolve 'node-request-interceptor' in '/Users/username/interceptors/test/intercept/fetch'

@esjs You should just need to run yarn && yarn build before running tests.

[esjs]

@esjs You should just need to run yarn && yarn build before running tests.

Thanks @msutkowski , added. Not sure whether I correctly understood how tests work, but it looks correct for me.

[msutkowski]

Alright - here's what I've discovered in regards to this. Even though worker.start doesn't currently support intercepting XMLHttpRequest in a browser environment (may change per https://github.com/mswjs/msw/issues/663), we're still doing this, which is what is causing your behavior: https://github.com/mswjs/msw/blob/2b133436cd405a9ebf6d6882d5b8e47997b943f6/src/utils/deferNetworkRequestsUntil.ts#L9-L20.

@kettanaito Being that the service worker doesn't interact with XMLHttpRequests, we should probably just drop this code unless there is some reason for it that I'm not understanding at this time?

[kettanaito]

Even though worker.start doesn't currently support intercepting XMLHttpRequest in a browser environment

XMLHttpRequests are intercepted by the worker just as fetch requests are. Did you discover some scenarios when they are not? MSW supports all types of requests in a browser, because the worker supports them.

Being that the service worker doesn't interact with XMLHttpRequests, we should probably just drop this code unless there is some reason for it that I'm not understanding at this time?

A good point, @msutkowski. We have something called waitUntilReady option that is enabled by default. It was designed to negate the race condition between the worker registering and your app making requests, so that deferred mounting wouldn't be the recommended way to start the worker in your app. This logic works on a premise that you call worker.start() in your root file, supposedly before rendering any application. The deferNetworkRequestsUntil utility would stub fetch/XHR and pend the requests that happen in this registration window until the worker is ready.

Note that you can opt-out of this behavior by setting waitUntilReady: false:

worker.start({ waitUntilReady: false })

However, taking all the considerations listed above, I wouldn't recommend that.

Let me apologize for my previous comment, I somehow assumed this issue was concerning Node.js, but it's clearly a browser scenario.

I'm not sure how we should handle synchronous XMLHttpRequests, as regardless of when they are dispatched, the worker registration may still be pending, causing a race condition. While eliminating that race condition, we are making the XMLHttpRequest instance asynchronous (see the until function that returns a Promise), and thus the order of execution shifts.

[esjs]

HI again @msutkowski @kettanaito .

I'm wondering, is there any solution would be provided to this issue and I'm just too impatient, or this is dead end and nothing can be done?

I understand that issue is due to asynchronous nature of ServiceWorkers and there is no good solution, but can you at least provide and option to disable MSW for synchronous request, or event better don't even try to intercept them in the first place?

Thanks!

[kettanaito]

@esjs, yes, I've included the temporary solution above:

worker.start({ waitUntilReady: false })

This is going to disable the built-in race condition handling and XMLHttpRequest will behave as usual.

As to whether this is a dead-end or not, I don't have enough information to tell. Considering that the nature of requests is asynchronous all the time (I'm treating sync XHR as an exception) and that the native deference of the requests is a good DX, I see little reason to address this as an issue per se. Disabling waitUntilReady sounds like a good solution for those cases when you wish to use synchronous XHR.

Alternatively, we could recommend using deferred mounting as the default setup, but I have a strong dislike for it, as it introduces some changes to the code unrelated to API mocking. There've been cases when waitUntilReady didn't accumulate the intermediate requests properly, but somehow there's no reproduction scenario for those cases.

[esjs]

I'm sorry, looks like I completely missed that part. It seems to work as expected with this option, thank you for your help @kettanaito . Should I close this issue and previously provided merge request for interceptors or they may be used layer?

[kettanaito]

Let's leave the interceptors pull request, as you've mentioned it does fail (although it shouldn't). It may be an unrelated thing, but still looks like an issue.