-
-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
Conversation
src/index.ts
Outdated
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 opted to make this an option and default it to false so as not to introduce a breaking change.
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 suggest drop this and make it true by default since you're introducing a breaking change anyway (feat:).
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 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.
tests/requests.test.ts
Outdated
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.
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?
I'm wondering, if having a permissive page.route() is causing performance issues, would it make sense to instead create page.route for every handler instead? Thinking out loud here. So nothing explicitly defined in your handlers even gets intercepted.
What do you think, @grxy?
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.
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.
- Is
page.route(/.+/, () => route.continue())slow for the same scenario? - Is there anything in your scenario that suggests
getResponse()becoming the root cause?
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.
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/vite@7.0.2_@types+node@22.15.29_jiti@2.4.2/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/vite@7.0.2_@types+node@22.15.29_jiti@2.4.2/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
getResponsescode? - Hundreds to thousands of these requests kicking off at once might be a source of resource contention
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.
We need to discuss this in more detail and profile this issue.
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'm wondering, if having a permissive page.route() is causing performance issues, would it make sense to instead create page.route for every handler instead? Thinking out loud here. So nothing explicitly defined in your handlers even gets intercepted.
I think that could be an option, and would make the plugin closer to ordinary playwright behavior, but it could be considered a divergence from MSW itself, which intercepts all the things by default. There would still be overhead in this case...but in the playwright matching logic rather than this plugin. Also, there could be potential issues translating MSW route matching logic to Playwright (I'm not sure if they are 1:1 today) so that could be a source of issues in the future. In the end, I think it probably comes down to a "product" decision, so it's up to you how you want the implementation to be architected.
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.
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/vite@7.0.2_@types+node@22.15.29_jiti@2.4.2/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/vite@7.0.2_@types+node@22.15.29_jiti@2.4.2/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
getResponsescode? - Hundreds to thousands of these requests kicking off at once might be a source of resource contention
@kettanaito Would like to get this back on your radar if you are available to review again. Thanks for taking a look!
Summary
This PR introduces a new
ignoreCommonAssetRequestsoption. When enabled, common asset requests are passed through and not intercepted by any provided MSW handlers.Details
ignoreCommonAssetRequestsboolean option tocreateNetworkFixtureandNetworkFixture.isCommonAssetRequestare allowed to pass through without interception.tests/requests.test.tsto verify both interception and pass-through behavior for asset requests.Motivation
This feature allows users to avoid intercepting common asset requests during testing, which can be expensive when one is using a vite development server.
Closes #13