Skip to content

feat: add ignoreCommonAssetRequests option #14

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: add ignoreCommonAssetRequests option #14

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5841b62
feat: add `ignoreCommonAssetRequests` option
grxy Jul 22, 2025
5db5185
chore: update `interceptCommonAssetRequests` default value based on f…
grxy Jul 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
RequestHandler,
WebSocketHandler,
getResponse,
isCommonAssetRequest,
} from 'msw'
import {
type WebSocketClientEventMap,
Expand All @@ -24,7 +25,8 @@ import {
} from '@mswjs/interceptors/WebSocket'

export interface CreateNetworkFixtureArgs {
initialHandlers: Array<RequestHandler | WebSocketHandler>
ignoreCommonAssetRequests?: boolean
initialHandlers?: Array<RequestHandler | WebSocketHandler>
}

/**
Expand Down Expand Up @@ -57,6 +59,7 @@ export function createNetworkFixture(
async ({ page }, use) => {
const worker = new NetworkFixture({
page,
ignoreCommonAssetRequests: args?.ignoreCommonAssetRequests ?? false,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to make this an option and default it to false so as not to introduce a breaking change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest drop this and make it true by default since you're introducing a breaking change anyway (feat:).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated to make the option true in 5db5185. Are you suggesting that we remove the option altogether? I'm hesitant to do that unless we implement the logic you suggested that only calls for each passed in MSW handler, since it would prevent mocking assets altogether in that case.

initialHandlers: args?.initialHandlers || [],
})

Expand All @@ -70,13 +73,16 @@ export function createNetworkFixture(

export class NetworkFixture extends SetupApi<LifeCycleEventsMap> {
#page: Page
#ignoreCommonAssetRequests: boolean

constructor(args: {
page: Page
ignoreCommonAssetRequests: boolean
initialHandlers: Array<RequestHandler | WebSocketHandler>
}) {
super(...args.initialHandlers)
this.#page = args.page
this.#ignoreCommonAssetRequests = args.ignoreCommonAssetRequests
}

public async start() {
Expand All @@ -88,6 +94,11 @@ export class NetworkFixture extends SetupApi<LifeCycleEventsMap> {
body: request.postDataBuffer(),
})

if (this.#ignoreCommonAssetRequests && isCommonAssetRequest(fetchRequest)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one thing I still don't quite understand. By ignoring requests here we are claiming that the performance degradation originates from the getResponse() logic below. But that is highly unlikely (unless you have millions of handlers, which isn't the case).

I believe we need to profile this problem better before merging a solution.

  1. Is page.route(/.+/, () => route.continue()) slow for the same scenario?
  2. Is there anything in your scenario that suggests getResponse() becoming the root cause?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is page.route(/.+/, () => route.continue()) slow for the same scenario?

I think this would be slower at scale (e.g. thousands of requests) due to unnecessary overhead, but not meaningful for fewer total requests.

Is there anything in your scenario that suggests getResponse() becoming the root cause?

getResponse seems to be a contributing factor, but is not necessarily the only cause. I think this really is a scaling problem, on the order of # of requests X # of handlers X request overhead. If a test is making lots of requests (in my case thousands of vite requests), a few ms of overhead for each of these adds up quickly.

I did some not-very-scientific profiling, and here are the results I am seeing:

With ignoreCommonAssetRequests === false:

Setup (http://localhost:5173/) took 6.999666999999931ms
Setup (http://localhost:5173/@vite/client) took 4.065000000000055ms
Setup (http://localhost:5173/node_modules/.pnpm/[email protected]_@[email protected][email protected]/node_modules/vite/dist/client/env.mjs) took 2.3236670000000004ms

With ignoreCommonAssetRequests === true:

Setup (http://localhost:5173/) took 4.779042000000004ms
Setup (http://localhost:5173/@vite/client) took 0.9514169999999922ms
Setup (http://localhost:5173/node_modules/.pnpm/[email protected]_@[email protected][email protected]/node_modules/vite/dist/client/env.mjs) took 0.771541999999954ms

Setup here represents the cost of executing the code before route.continue() is eventually called. This was done with 25 placeholder MSW handlers.

My conclusions/suspicions are as follows:

  • More handlers === more potential overhead for each request
  • Intercepted, but then continued requests are the most expensive
  • Something about vite request matching is expensive...maybe the URLs are expensive to match in the underlying getResponses code?
  • Hundreds to thousands of these requests kicking off at once might be a source of resource contention

route.continue()
return
}

const response = await getResponse(
this.handlersController.currentHandlers().filter((handler) => {
return handler instanceof RequestHandler
Expand Down
40 changes: 39 additions & 1 deletion tests/requests.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { test as testBase, expect } from '@playwright/test'
import { http } from 'msw'
import { http, HttpResponse } from 'msw'
import { createNetworkFixture, type NetworkFixture } from '../src/index.js'

interface Fixtures {
Expand Down Expand Up @@ -107,3 +107,41 @@ test('intercepts a POST request with array buffer body', async ({
expect(request.url).toBe('http://localhost:5173/action')
await expect(request.text()).resolves.toBe('hello world')
})

test('intercepts an asset request when interceptCommonAssetRequests is false', async ({ network, page }) => {
network.use(
http.get('/index.html', () => {
return HttpResponse.text('NOT VALID HTML')
}),
)

await page.goto('/')
const htmlContent = await page.evaluate(async () => {
const res = await fetch('/index.html')
return res.text()
})

expect(htmlContent).toContain('NOT VALID HTML')
})

const testIgnoreAssets = testBase.extend<Fixtures>({
network: createNetworkFixture({ ignoreCommonAssetRequests: true }),
})

testIgnoreAssets('passes through an asset request when interceptCommonAssetRequests is true', async ({ network, page }) => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One downside of enabling this behavior is that asset requests can no longer be mocked at all. Is that acceptable? Maybe there's a way we can modify the API of network.use to allow overriding default fixture options as well as passing handlers?

network.use(
http.get('/index.html', () => {
return HttpResponse.text('NOT VALID HTML')
}),
)

await page.goto('/')
const htmlContent = await page.evaluate(async () => {
const res = await fetch('/index.html')
return res.text()
})

expect(htmlContent).toContain('DOCTYPE html')
})