Possible race when patching window.fetch on start/stop/start

[jkieboom]

Asking as a discussion because I don't have a standalone repro case yet. I'm running into an issue where the window.fetch patching will run on top of an already patched fetch (from msw itself):

msw/src/utils/deferNetworkRequestsUntil.ts

Lines 23 to 28 in 115b435

	const originalFetch = window.fetch
	window.fetch = async (...args) => {
	await until(() => predicatePromise)
	window.fetch = originalFetch
	return window.fetch(...args)
	}

I'm setting up MSW in beforeEach (teardown in afterEach) and if a test does not make any network request that would wait for MSW to be setup, it is possible for the fetch patching to happen twice. It looks like stop does not "abort" the network request deferral.

I'm currently working around the issue by polling for "window.fetch" to be the native fetch again, after calling stop, but I feel this is a bug in msw fundamentally.

[kettanaito]

Hi, @jkieboom. That's an interesting observation. To clarify: we're speaking about running MSW in the browser, correct?

It looks like we should halt any deferred requests if the user calls worker.stop() and restore the window.fetch. We may use a Symbol set on window.fetch to detect and revert the patch.

We should also add an escape logic in worker.start() to check if window.fetch has already been patched and don't do that again. This will guard the library against patching the function multiple times.

[jkieboom]

To clarify: we're speaking about running MSW in the browser, correct?

Yes, forgot to mention that.

We should also add an escape logic in worker.start() to check if window.fetch has already been patched and don't do that again. This will guard the library against patching the function multiple times.

Hmm, but then it would wait for the wrong predicate promise?

[kettanaito]

No, I don't think so. Just do something like

if (!window.fetch[IS_PATCHED]) {
  window.fetch = () => {}
  window.fetch[IS_PATCHED] = true
}

To prevent repetitive patching of the function, and then some sort of cancellation logic for the pending promise when called worker.stop(), if necessary? What do you think?

[jkieboom]

My worry is that if the old patch is still in place, it will wait only for the old predicate, which is the old/previous worker registration. I don't know enough about the architecture of msw to know if this is an issue.

My local workaround is to make stop async and wait/poll until it's actually cleaned up correctly (by checking whether fetch is again the global fetch). Would it possible to make stop in msw async and wait for the cleanup?