feat: use the network source architecture (defineNetwork)#2650
feat: use the network source architecture (defineNetwork)#2650kettanaito wants to merge 99 commits intomainfrom
defineNetwork)#2650Conversation
720e665 to
f65e91d
Compare
defineNetwork APIdefineNetwork API
1298da4 to
336a1bb
Compare
336a1bb to
7e2bf32
Compare
defineNetwork APIdefineNetwork API
|
Opened #2676 as a focused follow-up for the It widens the shared Verified with:
|
Co-authored-by: Felmon Fekadu <felmonon@gmail.com> Co-authored-by: Artem Zakharchenko <kettanaito@gmail.com>
Colorless
|
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
.github/workflows/ci.yml (4)
10-11:⚠️ Potential issue | 🟠 MajorJob
build-20now runs on Node 22, losing Node 20 build coverage.The job is named
build-20and labeled "build (20)", butnode-versionis set to 22. This defeats the purpose of having separate build jobs for different Node versions. The cache key at line 41 still referencesnode-20, which is now misleading.If Node 20 compatibility testing is still required, revert this job to use
node-version: 20. Otherwise, rename the job tobuild-22and consolidate with the existingbuild-22job.🐛 Proposed fix to restore Node 20 testing
- name: Set up Node.js uses: actions/setup-node@v4 with: - node-version: 22 + node-version: 20 cache: 'pnpm'Also applies to: 25-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 10 - 11, The CI job named build-20 is configured to run on Node 22 (node-version: 22) while still using cache keys labeled node-20, causing misleading coverage; either change node-version back to 20 in the build-20 job and update its cache keys to match, or rename the job to build-22 (and update the job name/labels and cache key prefixes) and remove or consolidate the duplicate build-22 job; also apply the same fix pattern to the related jobs referenced as 25-26 so job names, node-version entries, and cache key prefixes stay consistent.
182-199:⚠️ Potential issue | 🟡 MinorJob name "test (e2e) (20)" suggests Node 20 but runs on Node 22.
The
test-e2ejob is labeled "test (e2e) (20)" but usesnode-version: 22. Either restore Node 20 for consistency with the job name or update the job name to reflect the actual Node version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 182 - 199, The GitHub Actions job "test-e2e" has a mismatched label vs runtime: the job's name string "test (e2e) (20)" does not match the configured Node version (actions/setup-node node-version: 22); update either the name to reflect Node 22 (e.g., change the name value used in the job) or change the node-version from 22 back to 20 so the label and runtime are consistent (refer to the "test-e2e" job, the name field "test (e2e) (20)", and the actions/setup-node with node-version).
80-97:⚠️ Potential issue | 🟡 Minor
test-unitdepends onbuild-20but runs on Node 22.The
test-unitjob depends onbuild-20and restores a cache keyed withnode-20, but now runs on Node 22. Ifbuild-20is fixed to use Node 20, this job should also use Node 20 for consistency, or explicitly depend on the Node 22 build.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 80 - 97, The test-unit job currently declares needs: build-20 but sets actions/setup-node@v4 with node-version: 22, causing a mismatch with the build that restores a node-20 cache; update the test-unit job (job name: test-unit) to use node-version: 20 in the actions/setup-node step to match the build-20 cache, or if you intend to run tests on Node 22 instead, change the dependency from needs: build-20 to the appropriate build job that ran on Node 22 and ensure the cache key/restore logic aligns with that Node version.
116-117:⚠️ Potential issue | 🟠 MajorJob
test-node-20now runs on Node 22, losing Node 20 test coverage.The job is named
test-node-20and labeled "test (node.js) (20)", but the runtime is Node 22. Combined with the build job issue, this means no Node 20 testing occurs despite the job naming suggesting otherwise.🐛 Proposed fix to restore Node 20 testing
- name: Set up Node.js uses: actions/setup-node@v4 with: - node-version: 22 + node-version: 20 cache: 'pnpm'Also applies to: 132-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 116 - 117, The CI job named test-node-20 is configured to run Node 22 despite its name/label; update the job's Node runtime configuration (e.g., the actions/setup-node node-version or the matrix value used by the test-node-20 job) to use node version 20 instead of 22, and make the same correction for the duplicate occurrence referenced around the other block (the second test-node-20 occurrence at the later lines) so the job name, label, and runtime all consistently run Node 20.config/replaceCoreImports.js (1)
1-10:⚠️ Potential issue | 🟡 MinorPotential stateful regex issue with
test()and the global flag.The
hasCoreImportsfunction callstest()on a regex that has theg(global) flag. Whentest()is called on a global regex, it updateslastIndex, which can cause subsequent calls to the same regex to start matching from that position rather than from the beginning. This can lead to inconsistent results if the same regex is reused.Since
replaceCoreImportsalso uses the same regex pattern, and they share the same regex objects (CORE_ESM_IMPORT_PATTERN/CORE_CJS_IMPORT_PATTERN), callinghasCoreImportsbeforereplaceCoreImportscould affect the replacement behavior.🔧 Suggested fix: Reset lastIndex or create new regex instances
function getCoreImportPattern(isEsm) { - return isEsm ? CORE_ESM_IMPORT_PATTERN : CORE_CJS_IMPORT_PATTERN + // Return new instances to avoid global regex state issues + return isEsm + ? /from ["'](`#core`(.*))["'](;)?/gm + : /require\(["'](`#core`(.*))["']\)(;)?/gm }Alternatively, reset
lastIndexbefore use:export function hasCoreImports(fileContents, isEsm) { - return getCoreImportPattern(isEsm).test(fileContents) + const pattern = getCoreImportPattern(isEsm) + pattern.lastIndex = 0 + return pattern.test(fileContents) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/replaceCoreImports.js` around lines 1 - 10, hasCoreImports uses shared regexes CORE_ESM_IMPORT_PATTERN/CORE_CJS_IMPORT_PATTERN that were created with the global flag, so successive test() calls mutate lastIndex and cause flaky results; fix by ensuring you use fresh regex instances or reset lastIndex before testing/replacing: update getCoreImportPattern to return a new RegExp based on the same pattern/flags (or explicitly set getCoreImportPattern(...).lastIndex = 0 before calling test) and apply the same approach in replaceCoreImports so both hasCoreImports and replaceCoreImports operate on non-stateful regexes.src/core/ws/utils/attachWebSocketLogger.ts (1)
85-96:⚠️ Potential issue | 🟡 MinorMissing abort signal on nested
server.addEventListener('message', ...).The inner
messageevent listener added toserver(line 88-90) does not use{ signal: controller.signal }. When the cleanup function is called, this listener will remain attached, potentially causing a memory leak or stale logging.🔧 Proposed fix
server.addEventListener( 'open', () => { - server.addEventListener('message', (event) => { - logIncomingServerMessage(event) - }) + server.addEventListener( + 'message', + (event) => { + logIncomingServerMessage(event) + }, + { signal: controller.signal }, + ) }, { once: true, signal: controller.signal, }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ws/utils/attachWebSocketLogger.ts` around lines 85 - 96, The nested server.addEventListener('message', ...) inside the 'open' handler is missing the abort signal so it won't be removed on cleanup; update the inner registration (the listener that calls logIncomingServerMessage) to pass the same { signal: controller.signal } option (or use a named handler and call server.removeEventListener(...) on cleanup) so the message listener is unregistered when controller.signal aborts; locate this in the block that registers the 'open' event and modify the inner server.addEventListener call accordingly.
🟡 Minor comments (5)
src/core/experimental/request-utils.ts-22-38 (1)
22-38:⚠️ Potential issue | 🟡 MinorRegex does not handle
msw/passthroughappearing first in a multi-value Accept header.The pattern
/(,\s+)?msw\/passthrough/only matches an optional comma before the token. If the Accept header is"msw/passthrough, application/json", the result will be", application/json"with a leading comma.Consider handling both positions:
Proposed fix
- const nextAcceptHeader = acceptHeader.replace(/(,\s+)?msw\/passthrough/, '') + const nextAcceptHeader = acceptHeader + .replace(/msw\/passthrough,\s*/, '') + .replace(/(,\s+)?msw\/passthrough/, '')Alternatively, a single regex with alternation:
- const nextAcceptHeader = acceptHeader.replace(/(,\s+)?msw\/passthrough/, '') + const nextAcceptHeader = acceptHeader.replace(/(,\s*)?msw\/passthrough(,\s*)?/, (_, before, after) => (before && after ? ', ' : ''))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/experimental/request-utils.ts` around lines 22 - 38, The accept-token removal regex in deleteRequestPassthroughHeader doesn't handle msw/passthrough when it's the first value; update the replacement to remove the token plus any surrounding commas/whitespace regardless of position by using a regex like /(?:^|\s*,\s*)msw\/passthrough(?:\s*,\s*|$)/ for the replace in deleteRequestPassthroughHeader (i.e., change how nextAcceptHeader is computed), then normalize the resulting header string by trimming and collapsing any leftover leading/trailing commas/extra whitespace before setting or deleting the accept header.src/core/experimental/on-unhandled-frame.test.ts-302-302 (1)
302-302:⚠️ Potential issue | 🟡 MinorFix two test titles that reference the wrong frame type.
Line 302 and Line 332 say “HTTP frame” but both tests exercise
TestWebSocketFrame. Rename to avoid confusion in CI failure output.✏️ Proposed rename
-it('supports printing the default warning in the custom callback for the HTTP frame', async () => { +it('supports printing the default warning in the custom callback for the WebSocket frame', async () => {-it('supports printing the default error in the custom callback for the HTTP frame', async () => { +it('supports printing the default error in the custom callback for the WebSocket frame', async () => {Also applies to: 332-332
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/experimental/on-unhandled-frame.test.ts` at line 302, Two test titles incorrectly say "HTTP frame" while exercising TestWebSocketFrame; update the titles to reference WebSocket/TestWebSocketFrame to avoid confusion. Locate the two it(...) blocks whose current descriptions begin with "supports printing the default warning in the custom callback for the HTTP frame" and change them to "supports printing the default warning in the custom callback for the WebSocket frame (TestWebSocketFrame)" (or similar wording that includes TestWebSocketFrame), ensuring both occurrences are updated so CI output correctly identifies the tested frame type.src/core/experimental/frames/websocket-frame.test.ts-182-182 (1)
182-182:⚠️ Potential issue | 🟡 MinorTypo in error message: "exceptin" should be "exception".
Proposed fix
- const exception = new Error('Unhandled exceptin') + const exception = new Error('Unhandled exception')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/experimental/frames/websocket-frame.test.ts` at line 182, Fix the typo in the test error message by changing the string passed to the Error constructor for the variable named exception from "Unhandled exceptin" to "Unhandled exception" in the websocket-frame.test; locate the declaration const exception = new Error('Unhandled exceptin') and update the message text only so the test error message is spelled correctly.src/core/experimental/handlers-controller.ts-49-53 (1)
49-53:⚠️ Potential issue | 🟡 Minor
currentHandlers()now reorders mixed handler lists.Flattening
Object.values(this.getState().handlers)groups handlers bykind, solistHandlers()no longer reflects the order passed tosetup*(),use(), orresetHandlers()when HTTP and WebSocket handlers are interleaved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/experimental/handlers-controller.ts` around lines 49 - 53, currentHandlers() flattens handlers by kind and thus reorders mixed HTTP/WebSocket registrations; preserve original registration order by changing state to track insertion sequence and returning handlers in that sequence: add an ordered list (e.g., state.orderedHandlers) that setup*(), use(), and resetHandlers() update on add/remove, and modify currentHandlers() to iterate state.orderedHandlers to collect non-null handlers (instead of Object.values(...).flat()), or alternately store a registration timestamp on each handler and sort by it in currentHandlers(); reference currentHandlers(), setup*(), use(), and resetHandlers() when making these changes.src/core/experimental/on-unhandled-frame.ts-18-21 (1)
18-21:⚠️ Potential issue | 🟡 Minor
defaults.warn/errorare async, not sync.Both defaults are bound to
printStrategyMessage()(line 27), an async function that awaitsframe.getUnhandledMessage()(line 34). The type declaration at lines 18-21 declares them as() => void, but the actual implementation returnsPromise<void>. This prevents usingawait defaults.warn()in custom callbacks and makes the defaults fire-and-forget by default.Suggested fix
export type UnhandledFrameDefaults = { - warn: () => void - error: () => void + warn: () => Promise<void> + error: () => Promise<void> }Also applies to: 27-45, 84-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/experimental/on-unhandled-frame.ts` around lines 18 - 21, The UnhandledFrameDefaults type and usages declare warn/error as synchronous but the implementation (printStrategyMessage, which awaits frame.getUnhandledMessage) returns Promise<void>; update the UnhandledFrameDefaults type so warn and error are typed as () => Promise<void>, and adjust any places that construct or call defaults.warn/defaults.error (e.g., where defaults are created and where they are invoked in printStrategyMessage and other call sites around lines 27-45 and 84-91) to handle/await the returned Promise accordingly (or explicitly return the Promise) so callers can await these handlers.
🧹 Nitpick comments (15)
test/node/rest-api/request/body/body-form-data.node.test.ts (1)
50-54: Reconsideringexpect.soft()for status assertions.Using
expect.soft()for the HTTP status check means the test will continue to the JSON body assertion even if the status is wrong. If the response isn't 200, the body assertion at line 51 will likely fail with a confusing error (e.g., an error response body instead of the expected entries), masking the actual root cause.If the intent is to see all failures, consider whether a failing status check should still proceed to body assertions, or if a hard failure is more appropriate here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/node/rest-api/request/body/body-form-data.node.test.ts` around lines 50 - 54, The status assertion uses expect.soft(res.status) which allows the test to continue on non-200 responses and can produce misleading body-failure noise; change the status check to a hard assertion (expect(res.status).toBe(200)) so the test immediately fails on unexpected HTTP status and prevents subsequent json assertions from running, updating the assertion that references res.status in this test file (body-form-data.node.test.ts) accordingly.test/browser/msw-api/setup-worker/scenarios/iframe-isolated-response/iframe-isolated-response.test.ts (1)
64-66: Redundant null checks after function type checks.These
!= nullchecks are unnecessary. Lines 59-62 already wait fortypeof window.request === 'function', which guarantees the value is neithernullnorundefined. A function is always truthy and non-null.🧹 Proposed fix to remove redundant checks
await Promise.all([ frameOne.waitForFunction(() => typeof window.request === 'function'), frameTwo.waitForFunction(() => typeof window.request === 'function'), ]) - await frameOne.waitForFunction(() => window.request != null) - await frameTwo.waitForFunction(() => window.request != null) - await Promise.all([🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/browser/msw-api/setup-worker/scenarios/iframe-isolated-response/iframe-isolated-response.test.ts` around lines 64 - 66, The two redundant waits using frameOne.waitForFunction(() => window.request != null) and frameTwo.waitForFunction(() => window.request != null) should be removed because earlier waits already assert typeof window.request === 'function' (so request cannot be null); delete these two lines (the calls to waitForFunction that check != null) to avoid duplicate checks and keep only the prior typeof checks that guarantee a function exists..github/workflows/ci.yml (2)
250-267:test-nativejob changed to Node 22.Same as other jobs depending on
build-20—verify alignment once the Node 20/22 split strategy is clarified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 250 - 267, The test-native job currently sets node-version: 22 but declares needs: build-20, causing a Node version mismatch; update the test-native job (look for the test-native job block and its node-version key) to align with the intended strategy—either change node-version to 20 to match build-20 or update the dependent build job and any other jobs to use Node 22 consistently—ensure the node-version value is consistent across jobs that share the same build dependency.
207-224:test-browserjob changed to Node 22.This job depends on
build-20and uses cache keys referencingnode-20. If Node 20 testing is restored for the build job, ensure this job's Node version and dependencies are aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 207 - 224, The test-browser job now uses Node 22 while it depends on build-20 and cache keys still reference node-20; update the test-browser job (job name: test-browser) to align Node version and cache with the build job by setting uses: actions/setup-node@v4 with node-version matching the build (e.g., 20) and ensure the pnpm cache key or related references that mention node-20 are consistent, or conversely update build-20 and its cache keys to Node 22 if you intend to move both jobs to Node 22 so both job names and cache keys remain in sync (refer to job names test-browser and build-20 and the cache/key entries).src/core/experimental/request-utils.test.ts (1)
49-69: Consider adding test coverage for multi-value Accept headers.The current tests only cover a single-value
accept: 'msw/passthrough'header. Consider adding a test for whenmsw/passthroughappears alongside other accept values (e.g.,'application/json, msw/passthrough') to verify the header is correctly trimmed while preserving other values.Suggested additional test
it('removes the bypass header while preserving other accept values', () => { const request = new Request('http://example.com', { headers: { accept: 'application/json, msw/passthrough', }, }) deleteRequestPassthroughHeader(request) expect(request.headers.get('accept')).toBe('application/json') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/experimental/request-utils.test.ts` around lines 49 - 69, Add a new unit test in src/core/experimental/request-utils.test.ts that verifies deleteRequestPassthroughHeader correctly removes the "msw/passthrough" token from a multi-value Accept header while preserving other values; create a Request with headers.accept set to a comma-separated string like "application/json, msw/passthrough", call deleteRequestPassthroughHeader(request), and assert that request.headers.get('accept') equals "application/json" (trimmed of whitespace as needed) to ensure multi-value handling is covered for the deleteRequestPassthroughHeader function.test/support/ws-test-utils.ts (2)
27-31: Type assertions may mask compatibility issues.The
as anycasts onsocketandtransportbypass type checking. If theWebSocketServerConnectionconstructor signature changes, these tests won't catch the mismatch at compile time. Consider adding a brief comment explaining why the casts are necessary (e.g., test environment limitations).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/support/ws-test-utils.ts` around lines 27 - 31, The test is using unsafe casts for socket and transport when instantiating WebSocketServerConnection which masks type mismatches; update the instantiation around WebSocketServerConnection to either use correctly typed test doubles/mocks for socket and transport or, if the test environment prevents that, replace the two "as any" casts with a short inline comment explaining why the casts are required (e.g., test harness types differ from runtime types) and reference the variables socket and transport so future maintainers know the intentional tradeoff; prefer creating minimally-typed stubs matching the constructor signature over using any when feasible.
1-9: Consolidate duplicate imports from the same module.The two import statements from
@mswjs/interceptors/WebSocketcan be merged into a single statement.Suggested consolidation
-import { - WebSocketClientConnection, - WebSocketConnectionData, - WebSocketServerConnection, -} from '@mswjs/interceptors/WebSocket' -import { - WebSocketTransport, - WebSocketData, -} from '@mswjs/interceptors/WebSocket' +import { + WebSocketClientConnection, + WebSocketConnectionData, + WebSocketServerConnection, + WebSocketTransport, + WebSocketData, +} from '@mswjs/interceptors/WebSocket'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/support/ws-test-utils.ts` around lines 1 - 9, The file has duplicate import statements from the same module; consolidate them into a single import from '@mswjs/interceptors/WebSocket' that imports WebSocketClientConnection, WebSocketConnectionData, WebSocketServerConnection, WebSocketTransport, and WebSocketData together to remove redundancy and keep imports tidy.src/core/ws/handleWebSocketEvent.ts (1)
20-24: Resolve this TODO by switching to kind-indexed lookup.Line 24 still does a per-connection filter over all handlers. Since handler maps are now kind-grouped, using controller-level
getHandlersByKind('websocket')here would remove repeated O(n) scans.I can draft the exact interface + call-site patch for this file and its options wiring if you want me to open a follow-up issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ws/handleWebSocketEvent.ts` around lines 20 - 24, The current code in handleWebSocketEvent.ts builds handlers by filtering all handlers via options.getHandlers().filter(isHandlerKind('websocket')), causing an O(n) scan per connection; replace that with the kind-indexed lookup by calling options.getHandlersByKind('websocket') (or the controller method exposed on options) and assign its result to the handlers variable, removing the isHandlerKind filter usage; ensure any call-sites expecting an array still work and that the options object exposes getHandlersByKind so the new call returns the pre-grouped websocket handlers.src/core/experimental/sources/network-source.test.ts (1)
21-24: Extract repeatedCustomNetworkSourcetest fixture.Same inline class appears in multiple tests; a small helper keeps this suite tighter and easier to evolve.
♻️ Optional cleanup
+class TestNetworkSource extends NetworkSource { + enable = async () => {} +} + it('emits the "frame" event when a frame is queued', async () => { - class CustomNetworkSource extends NetworkSource { - enable = async () => {} - } - - const source = new CustomNetworkSource() + const source = new TestNetworkSource() const frameListener = vi.fn() source.on('frame', frameListener) @@ it('removes all listeners when "disable" is called', async () => { - class CustomNetworkSource extends NetworkSource { - enable = async () => {} - } - - const source = new CustomNetworkSource() + const source = new TestNetworkSource() @@ it('accepts AbortSignal when attaching event listeners', async () => { - class CustomNetworkSource extends NetworkSource { - enable = async () => {} - } - - const source = new CustomNetworkSource() + const source = new TestNetworkSource()Also applies to: 38-40, 53-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/experimental/sources/network-source.test.ts` around lines 21 - 24, Extract the repeated inline test class CustomNetworkSource into a single shared fixture: create a top-level helper (e.g., a function or exported test-class) named createCustomNetworkSource or SharedCustomNetworkSource and replace each inline class declaration (instances at CustomNetworkSource in the three test sites) with calls to that helper; update tests that reference enable or instantiate the class to use the shared helper so the suite doesn't duplicate the same class definition across tests.src/core/experimental/compat.ts (1)
27-34: Consider setting HTTP method for WebSocket upgrade request.The synthetic
Requestfor WebSocket frames only setsconnectionandupgradeheaders. WebSocket upgrade requests are typically GET requests. While this may not matter for the legacy callback's use case, setting the method explicitly would be more accurate.💡 Optional enhancement
: frame instanceof WebSocketNetworkFrame ? new Request(frame.data.connection.client.url, { + method: 'GET', headers: { connection: 'upgrade', upgrade: 'websocket', }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/experimental/compat.ts` around lines 27 - 34, The synthetic Request created for WebSocket frames (the ternary branch that checks frame instanceof WebSocketNetworkFrame and constructs new Request(frame.data.connection.client.url, {...})) does not set an HTTP method; make it an explicit GET by adding method: 'GET' to the Request init object so the upgrade request accurately reflects a WebSocket handshake (keep the existing headers: connection: 'upgrade' and upgrade: 'websocket').test/browser/msw-api/setup-worker/stop.test.ts (1)
65-66: Clarify the purpose ofexpect.soft()here.Using
expect.soft()for thefromServiceWorker()assertion allows the test to continue even if this check fails. If this is intentional (e.g., the service worker behavior is secondary to the main assertion), this is fine. Otherwise, consider using a regularexpect()if both assertions are equally important.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/browser/msw-api/setup-worker/stop.test.ts` around lines 65 - 66, The use of expect.soft(response.fromServiceWorker()) is ambiguous; decide whether the fromServiceWorker() check is critical. If it is critical, replace expect.soft(response.fromServiceWorker()).toBe(true) with a regular expect(response.fromServiceWorker()).toBe(true) so the test fails immediately on mismatch; if the check is intentionally non-blocking, keep expect.soft but add a brief inline comment above that line explaining why the assertion is allowed to fail without aborting the test (e.g., "non-fatal check: service worker presence is optional for this scenario"). Ensure you reference the same call sites: expect.soft / expect and response.fromServiceWorker(), leaving the await expect(response.json()).resolves.toEqual({ original: true }) unchanged.test/browser/msw-api/setup-worker/start/on-unhandled-request/callback-print.test.ts (1)
70-70: Replace fixed 1s sleeps with deterministic synchronization.Line 70 and Line 149 add unconditional delays that slow the suite and can still be flaky. Prefer waiting on a concrete signal (specific console entry/network completion) or remove these waits if assertions already cover completion.
♻️ Proposed change
- await page.waitForTimeout(1000) + // No fixed delay needed; assertions above already await all expected side-effects.- await page.waitForTimeout(1000) + // No fixed delay needed; assertions above already await all expected side-effects.Also applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/browser/msw-api/setup-worker/start/on-unhandled-request/callback-print.test.ts` at line 70, Replace the unconditional page.waitForTimeout(1000) sleeps in callback-print.test.ts (instances at the call to page.waitForTimeout) with deterministic waits: wait for the specific console message using page.waitForEvent('console') filtered by message text, or wait for the network response with page.waitForResponse() that matches the request URL, or use page.waitForFunction() to poll a DOM/test-visible state that your assertions rely on; update both occurrences so assertions run only after these concrete signals instead of a fixed timeout.src/core/experimental/setup-api.ts (1)
30-37: WireeventsthroughpublicEmitter(or removepublicEmitter).
publicEmitteris created and disposed, but never used for subscriptions/emissions;eventsis bound directly toemitter. That makes the extra emitter dead state and weakens internal/public event separation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/experimental/setup-api.ts` around lines 30 - 37, The code creates both emitter and publicEmitter but binds this.events to emitter so publicEmitter is unused; either remove publicEmitter entirely or wire events through it: replace this.events = this.emitter with this.events = this.publicEmitter and then add forwarding so internal emits also publish to publicEmitter (e.g., in the constructor attach a forwarder from this.emitter to this.publicEmitter by calling this.emitter.on(event, (...args) => this.publicEmitter.emit(event, ...args)) for the relevant event names or by wrapping emitter.emit to call publicEmitter.emit as well); ensure the cleanup in this.subscriptions still removes listeners from both this.emitter and this.publicEmitter.src/browser/glossary.ts (1)
5-5: Consider using a type-only import forAnyHandler.
AnyHandleris only used in type positions within this file. Using a type import (import type) would make the intent clearer and could improve tree-shaking.Suggested fix
-import { AnyHandler } from '#core/experimental/handlers-controller' +import type { AnyHandler } from '#core/experimental/handlers-controller'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser/glossary.ts` at line 5, The import of AnyHandler in glossary.ts is used only as a type; change it to a type-only import to clarify intent and aid tree-shaking by replacing the current import of AnyHandler with a type import (e.g., use "import type { AnyHandler } ..." instead of the value import) while leaving other imports in the file unchanged and ensuring all references to AnyHandler remain as type annotations.src/core/experimental/sources/network-source.ts (1)
14-17: Theas anycast bypasses type safety in the super call.The spread with
as anysuggests a mismatch betweenTypedEventconstructor expectations and the arguments being passed. This could mask type errors ifTypedEvent's API changes.Consider aligning the constructor call with
TypedEvent's expected signature or documenting why the cast is necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/experimental/sources/network-source.ts` around lines 14 - 17, The constructor is bypassing type safety by using "as any" in the super call; update the NetworkSource constructor to call TypedEvent with the correct typed arguments instead of casting—inspect TypedEvent's constructor signature and either adjust NetworkSource's class generics (or the constructor parameter types) so you can call super(type, {}) with proper types, or explicitly construct the expected event payload type and pass that to super; remove the "as any" cast from the super(...([type, {}] as any)) call and ensure the constructor signature and class generics (and the AnyNetworkFrame usage) align with TypedEvent's expected types so the compiler enforces correctness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/scripts/patch-ts.js`:
- Around line 59-66: Add file-level ESLint directives to allow Node globals and
informational logging: at the very top of patch-ts.js add an ESLint env
directive and disable rules that block use of process and console (e.g., /*
eslint-env node */ and /* eslint-disable no-console, no-process-exit */) so the
uses of process.exit and console.log in this script (and other logging blocks)
are permitted by the linter.
In `@package.json`:
- Around line 175-177: The package.json currently only maps the exact import
"#core" which breaks all subpath imports like
"#core/experimental/handlers-controller" and "#core/utils/executeHandlers";
update the "imports" field to add a wildcard subpath mapping for "#core/*"
pointing to the source files (e.g., map "#core/*" -> "./src/core/*") while
keeping the existing "#core" entry so both exact and subpath imports resolve
correctly.
In `@src/browser/index.ts`:
- Line 1: The export in this module currently uses a type-only modifier for
SetupWorkerApi which breaks consumers that import it as a value; update the
export to re-export SetupWorkerApi as a regular export alongside setupWorker
(i.e., remove the "type" modifier from the export of SetupWorkerApi in the file
where setupWorker and SetupWorkerApi are re-exported), or if you intend strict
type-only semantics instead, update all consumer imports to use "import type {
SetupWorkerApi }" — target the export line that mentions setupWorker and
SetupWorkerApi and remove the "type" keyword so existing value-style imports
continue to work.
In `@src/browser/setup-worker.ts`:
- Around line 83-88: The early exit currently returns undefined directly which
violates the StartReturnType (Promise<ServiceWorkerRegistration | undefined>);
change the branch inside the start function so that when isStarted is true it
returns a resolved Promise (e.g., return Promise.resolve(undefined)) instead of
returning undefined, referencing the isStarted check and devUtils.warn in
setup-worker.ts to locate the code and preserve the declared return type.
- Around line 127-135: The stop() implementation fails to reset the isStarted
flag and ignores rejections from network.disable(); update stop() to set
isStarted = false once network.disable() completes (or immediately before/after
posting the 'msw/worker:stop' message) so start() can run again, and attach a
.catch handler (or use try/catch if async) to network.disable() to log or handle
errors instead of leaving the promise unhandled; ensure these changes reference
the existing stop(), isStarted, start(), and network.disable() symbols and
preserve the window.postMessage call.
In `@src/browser/sources/service-worker-source.ts`:
- Around line 69-71: The channel currently captures the original workerPromise
in the constructor (this.#channel = new WorkerChannel({ worker:
this.workerPromise.then(([worker]) => worker) })) so stop()/disable() that
replaces workerPromise leaves the old worker, keepalive timer and beforeunload
listener alive; update stop()/disable() to fully teardown the prior session by
calling a teardown/close on this.#channel (or nulling it), clearing any
keepalive timers, and removing the beforeunload listener, and change how
this.#channel is created/used so it resolves the current workerPromise on each
start (e.g., supply a function/getter or recreate WorkerChannel in start() using
the current this.workerPromise) so start()/stop()/start() cycles can't talk to a
stale worker or leak hooks.
- Around line 244-249: Move the removal of the frame so it always runs before
the opaque-response early return: call this.#frames.delete(request.id)
immediately after retrieving the frame (after const frame =
this.#frames.get(request.id)) and before the if
(response.type?.includes('opaque')) return; check so no stale entries remain for
no-cors/opaque responses; keep the existing this.#frames.get(...) usage and any
subsequent logic that depends on frame.
In `@src/core/experimental/define-network.ts`:
- Around line 166-172: disable() currently calls listenersController.abort() but
listenersController is only assigned in enable(), so calling disable() first
will throw; update disable() to guard against undefined (e.g., if
(listenersController) listenersController.abort()) or initialize
listenersController when the network object is created so it's always defined.
Ensure the symbol listenersController's type/signature is adjusted if needed and
leave the remainder of disable() (the colorlessPromiseAll call over
resolvedOptions.sources.map(source => source.disable())) unchanged.
In `@src/core/experimental/frames/http-frame.ts`:
- Line 143: The request lifecycle currently emits RequestEvent('request:start',
...) but when the handler lookup path calls errorWith() it exits without
emitting a RequestEvent('request:end'), leaking active-request state; update the
handler-lookup/error branch in http-frame.ts to always emit events.emit(new
RequestEvent('request:end', { requestId, request, error })) before returning
(mirror how successful paths emit 'request:end'), ensuring you capture the
thrown/error object and include requestId and request so consumers can clean up;
place this emission immediately before or inside the errorWith() return path so
both the normal and exceptional flows close the lifecycle.
- Around line 215-250: The code currently only emits the
RequestEvent('request:match') after confirming a non-passthrough response, which
hides explicit passthrough handlers; update the logic in the block handling
lookupResult (where you destructure const { response, handler, parsedResult } =
lookupResult) to emit RequestEvent('request:match', { requestId, request }) as
soon as lookupResult is non-null (before the checks for response == null and
isPassthroughResponse(response)), so matched-but-passthrough cases still produce
a 'request:match' event while keeping the existing calls to
storeResponseCookies, passthrough(), and RequestEvent('request:end') intact.
- Line 6: The override of resolve in HttpNetworkFrame narrows the parameter type
to Array<HttpHandler> but getHandlers() returns Array<AnyHandler> and the parent
NetworkFrame declares resolve(handlers: Array<AnyHandler>, ...); update
HttpNetworkFrame.resolve to accept handlers: Array<AnyHandler> (instead of
Array<HttpHandler>) so the signature matches the base class and callers can pass
the output of getHandlers(); similarly widen the parameter in
WebSocketNetworkFrame.resolve from Array<WebSocketHandler> to Array<AnyHandler>,
then adjust any internal type assertions/guards inside those resolve
implementations to narrow handlers to HttpHandler or WebSocketHandler before
using handler-specific properties.
In `@src/core/experimental/frames/websocket-frame.ts`:
- Around line 112-143: The code marks hasMatchingHandlers = true as soon as a
handler.run() yields a handlerConnection, but that should only happen if the
handler actually handled the connection via
handler[kConnect](handlerConnection); change the logic so you call
handler[kConnect](handlerConnection) first and set hasMatchingHandlers = true
only when that call returns truthy (and still ensure removeLogger is disposed
when the connect call returns false); apply the same change to the second
occurrence (the block that mirrors lines 169-179) so only handlers that
successfully run connection listeners flip hasMatchingHandlers.
In `@src/core/experimental/request-utils.test.ts`:
- Around line 28-39: The test for isPassthroughResponse is missing an assertion
and thus always passes; update the test so the expect(...) call on
isPassthroughResponse(new Response(..., { headers: {
[REQUEST_INTENTION_HEADER_NAME]: RequestIntention.passthrough } })) includes a
chained assertion such as .toBe(true) (or .toEqual(true)) so the test actually
verifies the function returns true.
In `@src/core/experimental/sources/interceptor-source.ts`:
- Around line 144-153: The errorWith method incorrectly falls through after
forwarding InternalError to this.#controller.errorWith, causing the same
InternalError to be rethrown; update errorWith (in interceptor-source.ts) so
that after calling this.#controller.errorWith(reason) it immediately returns
(similar to the Response branch) to prevent the extra throw and
double-rejection; make sure you still handle Response via respondWith and only
rethrow for other unknown reasons.
In `@src/core/handlers/RequestHandler.ts`:
- Line 135: The RequestHandler's public property "kind" is mutable and should be
made immutable to prevent accidental reassignment; update the RequestHandler
definition to expose an immutable discriminator (e.g., change the property
declaration on RequestHandler from "public kind = 'request' as const" to a
readonly declaration such as "public readonly kind = 'request' as const" or
implement a read-only getter "public get kind(): 'request' { return 'request'
}") so the handler-kind cannot be reassigned and routing/filtering invariants
remain intact.
In `@test/node/msw-api/setup-server/resetHandlers.node.test.ts`:
- Around line 91-97: The test calls fetch('http://localhost/books', { method:
'POST' }) which doesn't exercise the original handler registered with http.get,
so change the request to use GET (remove or set method to 'GET') so the fetch
targets the same method/path as the original handler; update the fetch
invocation near the hadError variable and keep the surrounding logic that
expects the request to fail after resetHandlers(...) to ensure the test actually
verifies the original GET /books handler was replaced.
In `@test/tsconfig.json`:
- Line 17: Update the tsconfig compilerOptions.types entry to use the package
name "node" instead of "@types/node": locate the compilerOptions.types array
(the "types" key in the tsconfig JSON) and replace the "@types/node" string with
"node" so TypeScript resolves Node globals correctly while keeping
"vitest/globals" as-is.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 10-11: The CI job named build-20 is configured to run on Node 22
(node-version: 22) while still using cache keys labeled node-20, causing
misleading coverage; either change node-version back to 20 in the build-20 job
and update its cache keys to match, or rename the job to build-22 (and update
the job name/labels and cache key prefixes) and remove or consolidate the
duplicate build-22 job; also apply the same fix pattern to the related jobs
referenced as 25-26 so job names, node-version entries, and cache key prefixes
stay consistent.
- Around line 182-199: The GitHub Actions job "test-e2e" has a mismatched label
vs runtime: the job's name string "test (e2e) (20)" does not match the
configured Node version (actions/setup-node node-version: 22); update either the
name to reflect Node 22 (e.g., change the name value used in the job) or change
the node-version from 22 back to 20 so the label and runtime are consistent
(refer to the "test-e2e" job, the name field "test (e2e) (20)", and the
actions/setup-node with node-version).
- Around line 80-97: The test-unit job currently declares needs: build-20 but
sets actions/setup-node@v4 with node-version: 22, causing a mismatch with the
build that restores a node-20 cache; update the test-unit job (job name:
test-unit) to use node-version: 20 in the actions/setup-node step to match the
build-20 cache, or if you intend to run tests on Node 22 instead, change the
dependency from needs: build-20 to the appropriate build job that ran on Node 22
and ensure the cache key/restore logic aligns with that Node version.
- Around line 116-117: The CI job named test-node-20 is configured to run Node
22 despite its name/label; update the job's Node runtime configuration (e.g.,
the actions/setup-node node-version or the matrix value used by the test-node-20
job) to use node version 20 instead of 22, and make the same correction for the
duplicate occurrence referenced around the other block (the second test-node-20
occurrence at the later lines) so the job name, label, and runtime all
consistently run Node 20.
In `@config/replaceCoreImports.js`:
- Around line 1-10: hasCoreImports uses shared regexes
CORE_ESM_IMPORT_PATTERN/CORE_CJS_IMPORT_PATTERN that were created with the
global flag, so successive test() calls mutate lastIndex and cause flaky
results; fix by ensuring you use fresh regex instances or reset lastIndex before
testing/replacing: update getCoreImportPattern to return a new RegExp based on
the same pattern/flags (or explicitly set getCoreImportPattern(...).lastIndex =
0 before calling test) and apply the same approach in replaceCoreImports so both
hasCoreImports and replaceCoreImports operate on non-stateful regexes.
In `@src/core/ws/utils/attachWebSocketLogger.ts`:
- Around line 85-96: The nested server.addEventListener('message', ...) inside
the 'open' handler is missing the abort signal so it won't be removed on
cleanup; update the inner registration (the listener that calls
logIncomingServerMessage) to pass the same { signal: controller.signal } option
(or use a named handler and call server.removeEventListener(...) on cleanup) so
the message listener is unregistered when controller.signal aborts; locate this
in the block that registers the 'open' event and modify the inner
server.addEventListener call accordingly.
---
Minor comments:
In `@src/core/experimental/frames/websocket-frame.test.ts`:
- Line 182: Fix the typo in the test error message by changing the string passed
to the Error constructor for the variable named exception from "Unhandled
exceptin" to "Unhandled exception" in the websocket-frame.test; locate the
declaration const exception = new Error('Unhandled exceptin') and update the
message text only so the test error message is spelled correctly.
In `@src/core/experimental/handlers-controller.ts`:
- Around line 49-53: currentHandlers() flattens handlers by kind and thus
reorders mixed HTTP/WebSocket registrations; preserve original registration
order by changing state to track insertion sequence and returning handlers in
that sequence: add an ordered list (e.g., state.orderedHandlers) that setup*(),
use(), and resetHandlers() update on add/remove, and modify currentHandlers() to
iterate state.orderedHandlers to collect non-null handlers (instead of
Object.values(...).flat()), or alternately store a registration timestamp on
each handler and sort by it in currentHandlers(); reference currentHandlers(),
setup*(), use(), and resetHandlers() when making these changes.
In `@src/core/experimental/on-unhandled-frame.test.ts`:
- Line 302: Two test titles incorrectly say "HTTP frame" while exercising
TestWebSocketFrame; update the titles to reference WebSocket/TestWebSocketFrame
to avoid confusion. Locate the two it(...) blocks whose current descriptions
begin with "supports printing the default warning in the custom callback for the
HTTP frame" and change them to "supports printing the default warning in the
custom callback for the WebSocket frame (TestWebSocketFrame)" (or similar
wording that includes TestWebSocketFrame), ensuring both occurrences are updated
so CI output correctly identifies the tested frame type.
In `@src/core/experimental/on-unhandled-frame.ts`:
- Around line 18-21: The UnhandledFrameDefaults type and usages declare
warn/error as synchronous but the implementation (printStrategyMessage, which
awaits frame.getUnhandledMessage) returns Promise<void>; update the
UnhandledFrameDefaults type so warn and error are typed as () => Promise<void>,
and adjust any places that construct or call defaults.warn/defaults.error (e.g.,
where defaults are created and where they are invoked in printStrategyMessage
and other call sites around lines 27-45 and 84-91) to handle/await the returned
Promise accordingly (or explicitly return the Promise) so callers can await
these handlers.
In `@src/core/experimental/request-utils.ts`:
- Around line 22-38: The accept-token removal regex in
deleteRequestPassthroughHeader doesn't handle msw/passthrough when it's the
first value; update the replacement to remove the token plus any surrounding
commas/whitespace regardless of position by using a regex like
/(?:^|\s*,\s*)msw\/passthrough(?:\s*,\s*|$)/ for the replace in
deleteRequestPassthroughHeader (i.e., change how nextAcceptHeader is computed),
then normalize the resulting header string by trimming and collapsing any
leftover leading/trailing commas/extra whitespace before setting or deleting the
accept header.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 250-267: The test-native job currently sets node-version: 22 but
declares needs: build-20, causing a Node version mismatch; update the
test-native job (look for the test-native job block and its node-version key) to
align with the intended strategy—either change node-version to 20 to match
build-20 or update the dependent build job and any other jobs to use Node 22
consistently—ensure the node-version value is consistent across jobs that share
the same build dependency.
- Around line 207-224: The test-browser job now uses Node 22 while it depends on
build-20 and cache keys still reference node-20; update the test-browser job
(job name: test-browser) to align Node version and cache with the build job by
setting uses: actions/setup-node@v4 with node-version matching the build (e.g.,
20) and ensure the pnpm cache key or related references that mention node-20 are
consistent, or conversely update build-20 and its cache keys to Node 22 if you
intend to move both jobs to Node 22 so both job names and cache keys remain in
sync (refer to job names test-browser and build-20 and the cache/key entries).
In `@src/browser/glossary.ts`:
- Line 5: The import of AnyHandler in glossary.ts is used only as a type; change
it to a type-only import to clarify intent and aid tree-shaking by replacing the
current import of AnyHandler with a type import (e.g., use "import type {
AnyHandler } ..." instead of the value import) while leaving other imports in
the file unchanged and ensuring all references to AnyHandler remain as type
annotations.
In `@src/core/experimental/compat.ts`:
- Around line 27-34: The synthetic Request created for WebSocket frames (the
ternary branch that checks frame instanceof WebSocketNetworkFrame and constructs
new Request(frame.data.connection.client.url, {...})) does not set an HTTP
method; make it an explicit GET by adding method: 'GET' to the Request init
object so the upgrade request accurately reflects a WebSocket handshake (keep
the existing headers: connection: 'upgrade' and upgrade: 'websocket').
In `@src/core/experimental/request-utils.test.ts`:
- Around line 49-69: Add a new unit test in
src/core/experimental/request-utils.test.ts that verifies
deleteRequestPassthroughHeader correctly removes the "msw/passthrough" token
from a multi-value Accept header while preserving other values; create a Request
with headers.accept set to a comma-separated string like "application/json,
msw/passthrough", call deleteRequestPassthroughHeader(request), and assert that
request.headers.get('accept') equals "application/json" (trimmed of whitespace
as needed) to ensure multi-value handling is covered for the
deleteRequestPassthroughHeader function.
In `@src/core/experimental/setup-api.ts`:
- Around line 30-37: The code creates both emitter and publicEmitter but binds
this.events to emitter so publicEmitter is unused; either remove publicEmitter
entirely or wire events through it: replace this.events = this.emitter with
this.events = this.publicEmitter and then add forwarding so internal emits also
publish to publicEmitter (e.g., in the constructor attach a forwarder from
this.emitter to this.publicEmitter by calling this.emitter.on(event, (...args)
=> this.publicEmitter.emit(event, ...args)) for the relevant event names or by
wrapping emitter.emit to call publicEmitter.emit as well); ensure the cleanup in
this.subscriptions still removes listeners from both this.emitter and
this.publicEmitter.
In `@src/core/experimental/sources/network-source.test.ts`:
- Around line 21-24: Extract the repeated inline test class CustomNetworkSource
into a single shared fixture: create a top-level helper (e.g., a function or
exported test-class) named createCustomNetworkSource or
SharedCustomNetworkSource and replace each inline class declaration (instances
at CustomNetworkSource in the three test sites) with calls to that helper;
update tests that reference enable or instantiate the class to use the shared
helper so the suite doesn't duplicate the same class definition across tests.
In `@src/core/experimental/sources/network-source.ts`:
- Around line 14-17: The constructor is bypassing type safety by using "as any"
in the super call; update the NetworkSource constructor to call TypedEvent with
the correct typed arguments instead of casting—inspect TypedEvent's constructor
signature and either adjust NetworkSource's class generics (or the constructor
parameter types) so you can call super(type, {}) with proper types, or
explicitly construct the expected event payload type and pass that to super;
remove the "as any" cast from the super(...([type, {}] as any)) call and ensure
the constructor signature and class generics (and the AnyNetworkFrame usage)
align with TypedEvent's expected types so the compiler enforces correctness.
In `@src/core/ws/handleWebSocketEvent.ts`:
- Around line 20-24: The current code in handleWebSocketEvent.ts builds handlers
by filtering all handlers via
options.getHandlers().filter(isHandlerKind('websocket')), causing an O(n) scan
per connection; replace that with the kind-indexed lookup by calling
options.getHandlersByKind('websocket') (or the controller method exposed on
options) and assign its result to the handlers variable, removing the
isHandlerKind filter usage; ensure any call-sites expecting an array still work
and that the options object exposes getHandlersByKind so the new call returns
the pre-grouped websocket handlers.
In
`@test/browser/msw-api/setup-worker/scenarios/iframe-isolated-response/iframe-isolated-response.test.ts`:
- Around line 64-66: The two redundant waits using frameOne.waitForFunction(()
=> window.request != null) and frameTwo.waitForFunction(() => window.request !=
null) should be removed because earlier waits already assert typeof
window.request === 'function' (so request cannot be null); delete these two
lines (the calls to waitForFunction that check != null) to avoid duplicate
checks and keep only the prior typeof checks that guarantee a function exists.
In
`@test/browser/msw-api/setup-worker/start/on-unhandled-request/callback-print.test.ts`:
- Line 70: Replace the unconditional page.waitForTimeout(1000) sleeps in
callback-print.test.ts (instances at the call to page.waitForTimeout) with
deterministic waits: wait for the specific console message using
page.waitForEvent('console') filtered by message text, or wait for the network
response with page.waitForResponse() that matches the request URL, or use
page.waitForFunction() to poll a DOM/test-visible state that your assertions
rely on; update both occurrences so assertions run only after these concrete
signals instead of a fixed timeout.
In `@test/browser/msw-api/setup-worker/stop.test.ts`:
- Around line 65-66: The use of expect.soft(response.fromServiceWorker()) is
ambiguous; decide whether the fromServiceWorker() check is critical. If it is
critical, replace expect.soft(response.fromServiceWorker()).toBe(true) with a
regular expect(response.fromServiceWorker()).toBe(true) so the test fails
immediately on mismatch; if the check is intentionally non-blocking, keep
expect.soft but add a brief inline comment above that line explaining why the
assertion is allowed to fail without aborting the test (e.g., "non-fatal check:
service worker presence is optional for this scenario"). Ensure you reference
the same call sites: expect.soft / expect and response.fromServiceWorker(),
leaving the await expect(response.json()).resolves.toEqual({ original: true })
unchanged.
In `@test/node/rest-api/request/body/body-form-data.node.test.ts`:
- Around line 50-54: The status assertion uses expect.soft(res.status) which
allows the test to continue on non-200 responses and can produce misleading
body-failure noise; change the status check to a hard assertion
(expect(res.status).toBe(200)) so the test immediately fails on unexpected HTTP
status and prevents subsequent json assertions from running, updating the
assertion that references res.status in this test file
(body-form-data.node.test.ts) accordingly.
In `@test/support/ws-test-utils.ts`:
- Around line 27-31: The test is using unsafe casts for socket and transport
when instantiating WebSocketServerConnection which masks type mismatches; update
the instantiation around WebSocketServerConnection to either use correctly typed
test doubles/mocks for socket and transport or, if the test environment prevents
that, replace the two "as any" casts with a short inline comment explaining why
the casts are required (e.g., test harness types differ from runtime types) and
reference the variables socket and transport so future maintainers know the
intentional tradeoff; prefer creating minimally-typed stubs matching the
constructor signature over using any when feasible.
- Around line 1-9: The file has duplicate import statements from the same
module; consolidate them into a single import from
'@mswjs/interceptors/WebSocket' that imports WebSocketClientConnection,
WebSocketConnectionData, WebSocketServerConnection, WebSocketTransport, and
WebSocketData together to remove redundancy and keep imports tidy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| 'Found no .d.ts modules containing the "#core" import, skipping...', | ||
| ) | ||
| return process.exit(0) | ||
| } | ||
|
|
||
| console.log( | ||
| 'Found %d module(s) with the "~/core" import, resolving...', | ||
| 'Found %d module(s) with the "#core" import, resolving...', | ||
| typeDefsWithCoreImports.length, |
There was a problem hiding this comment.
Fix Node ESLint context for this script to avoid CI failures.
This file uses Node globals (process, console) and informational logs, but current linting flags them as errors. Add explicit Node/script lint directives at the file top so these checks don’t fail.
✅ Proposed fix
+/* eslint-env node */
+/* eslint-disable no-console */
import fs from 'node:fs'
import { exec } from 'node:child_process'
import { promisify } from 'node:util'Also applies to: 86-90, 145-145
🧰 Tools
🪛 ESLint
[error] 61-61: 'process' is not defined.
(no-undef)
[error] 64-64: 'console' is not defined.
(no-undef)
[error] 64-64: Unexpected console statement. Only these console methods are allowed: warn, error, group, groupCollapsed, groupEnd.
(no-console)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/scripts/patch-ts.js` around lines 59 - 66, Add file-level ESLint
directives to allow Node globals and informational logging: at the very top of
patch-ts.js add an ESLint env directive and disable rules that block use of
process and console (e.g., /* eslint-env node */ and /* eslint-disable
no-console, no-process-exit */) so the uses of process.exit and console.log in
this script (and other logging blocks) are permitted by the linter.
| "imports": { | ||
| "#core": "./src/core" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Current package imports map:"
jq '.imports' package.json
echo
echo "Searching for `#core` subpath imports (e.g. `#core/`...):"
rg -nP "(from\\s+['\"]#core/|import\\s*\\(\\s*['\"]#core/)" --type=ts --type=js --glob '!lib/**' --glob '!node_modules/**' || trueRepository: mswjs/msw
Length of output: 4722
Add #core/* mapping—subpath imports currently will fail resolution.
The package.json imports only defines the exact alias #core, but the codebase has 40+ active imports using #core/... subpaths across 15+ files (e.g., #core/experimental/handlers-controller, #core/utils/executeHandlers). These will fail module resolution without the subpath mapping.
✅ Proposed fix
"imports": {
- "#core": "./src/core"
+ "#core": "./src/core",
+ "#core/*": "./src/core/*"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "imports": { | |
| "#core": "./src/core" | |
| }, | |
| "imports": { | |
| "#core": "./src/core", | |
| "#core/*": "./src/core/*" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 175 - 177, The package.json currently only maps
the exact import "#core" which breaks all subpath imports like
"#core/experimental/handlers-controller" and "#core/utils/executeHandlers";
update the "imports" field to add a wildcard subpath mapping for "#core/*"
pointing to the source files (e.g., map "#core/*" -> "./src/core/*") while
keeping the existing "#core" entry so both exact and subpath imports resolve
correctly.
| export { setupWorker } from './setupWorker/setupWorker' | ||
| export type { SetupWorker, StartOptions } from './setupWorker/glossary' | ||
| export { SetupWorkerApi } from './setupWorker/setupWorker' | ||
| export { setupWorker, type SetupWorkerApi } from './setup-worker' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check how setup-worker declares SetupWorkerApi:"
ast-grep --pattern $'export class SetupWorkerApi { $$$ }' src/browser/setup-worker.ts || true
ast-grep --pattern $'export interface SetupWorkerApi { $$$ }' src/browser/setup-worker.ts || true
ast-grep --pattern $'export type SetupWorkerApi = $_' src/browser/setup-worker.ts || true
echo
echo "Find in-repo value-like usage of SetupWorkerApi:"
rg -nP '\b(new\s+SetupWorkerApi|instanceof\s+SetupWorkerApi|extends\s+SetupWorkerApi)\b' --type=ts --glob '!lib/**' || trueRepository: mswjs/msw
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the actual definition of SetupWorkerApi
echo "=== Search for SetupWorkerApi definition in setup-worker.ts ==="
rg -nA5 'SetupWorkerApi' src/browser/setup-worker.ts || echo "No matches in setup-worker.ts"
echo
echo "=== Check entire src/browser directory for SetupWorkerApi ==="
rg -nA2 'SetupWorkerApi' src/browser/ || echo "No matches in src/browser/"
echo
echo "=== Check for imports of SetupWorkerApi in codebase ==="
rg 'import.*SetupWorkerApi' --type=ts || echo "No imports found"
echo
echo "=== Check package.json exports ==="
cat package.json | grep -A 20 '"exports"' || trueRepository: mswjs/msw
Length of output: 4436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for CHANGELOG or migration notes ==="
rg -i 'type.*setupworkerapi|setupworkerapi.*breaking|setupworkerapi.*export' . --type=md -A 2 || echo "No changelog entries found"
echo
echo "=== Check git history for this line (recent commits) ==="
git log --oneline -n 20 -- src/browser/index.ts 2>/dev/null || echo "Git history not available"
echo
echo "=== Count value vs type imports in tests ==="
echo "Value imports (without type keyword):"
rg 'import\s*\{\s*SetupWorkerApi\s*\}' --type=ts | wc -l
echo "Type imports (with type keyword):"
rg 'import\s*(type\s+)?\{\s*type\s+SetupWorkerApi' --type=ts | wc -lRepository: mswjs/msw
Length of output: 393
Remove the type modifier to maintain backward compatibility with value imports.
Line 1 enforces type-only imports for SetupWorkerApi, but 16 test files currently use value imports (import { SetupWorkerApi }). This breaks existing consumer code. Since SetupWorkerApi is an interface (type-only at runtime), the type modifier adds strictness but is a breaking change without consumer migration.
Change to: export { setupWorker, SetupWorkerApi } from './setup-worker'
Or if type-only enforcement is intentional, update all consuming code to use import type { SetupWorkerApi }.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/browser/index.ts` at line 1, The export in this module currently uses a
type-only modifier for SetupWorkerApi which breaks consumers that import it as a
value; update the export to re-export SetupWorkerApi as a regular export
alongside setupWorker (i.e., remove the "type" modifier from the export of
SetupWorkerApi in the file where setupWorker and SetupWorkerApi are
re-exported), or if you intend strict type-only semantics instead, update all
consumer imports to use "import type { SetupWorkerApi }" — target the export
line that mentions setupWorker and SetupWorkerApi and remove the "type" keyword
so existing value-style imports continue to work.
| if (isStarted) { | ||
| devUtils.warn( | ||
| 'Found a redundant "worker.start()" call. Note that starting the worker while mocking is already enabled will have no effect. Consider removing this "worker.start()" call.', | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
Returning undefined instead of a Promise breaks the return type contract.
When isStarted is true, the method returns undefined directly, but the declared return type is StartReturnType (Promise<ServiceWorkerRegistration | undefined>). Callers expecting to chain .then() on the result will encounter a runtime error.
Proposed fix
if (isStarted) {
devUtils.warn(
'Found a redundant "worker.start()" call. Note that starting the worker while mocking is already enabled will have no effect. Consider removing this "worker.start()" call.',
)
- return
+ return Promise.resolve(undefined)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isStarted) { | |
| devUtils.warn( | |
| 'Found a redundant "worker.start()" call. Note that starting the worker while mocking is already enabled will have no effect. Consider removing this "worker.start()" call.', | |
| ) | |
| return | |
| } | |
| if (isStarted) { | |
| devUtils.warn( | |
| 'Found a redundant "worker.start()" call. Note that starting the worker while mocking is already enabled will have no effect. Consider removing this "worker.start()" call.', | |
| ) | |
| return Promise.resolve(undefined) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/browser/setup-worker.ts` around lines 83 - 88, The early exit currently
returns undefined directly which violates the StartReturnType
(Promise<ServiceWorkerRegistration | undefined>); change the branch inside the
start function so that when isStarted is true it returns a resolved Promise
(e.g., return Promise.resolve(undefined)) instead of returning undefined,
referencing the isStarted check and devUtils.warn in setup-worker.ts to locate
the code and preserve the declared return type.
| stop() { | ||
| if (!isStarted) { | ||
| return | ||
| } | ||
|
|
||
| network.disable().then(() => { | ||
| window.postMessage({ type: 'msw/worker:stop' }) | ||
| }) | ||
| }, |
There was a problem hiding this comment.
stop() doesn't reset isStarted, preventing valid restart.
After calling stop(), the isStarted flag remains true. A subsequent start() call will incorrectly warn about redundant start and return early without re-enabling the network.
Additionally, the promise rejection from network.disable() is unhandled.
Proposed fix
stop() {
if (!isStarted) {
return
}
- network.disable().then(() => {
+ isStarted = false
+ network.disable().then(() => {
window.postMessage({ type: 'msw/worker:stop' })
+ }).catch((error) => {
+ devUtils.warn('Failed to stop the worker: %s', error)
})
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/browser/setup-worker.ts` around lines 127 - 135, The stop()
implementation fails to reset the isStarted flag and ignores rejections from
network.disable(); update stop() to set isStarted = false once network.disable()
completes (or immediately before/after posting the 'msw/worker:stop' message) so
start() can run again, and attach a .catch handler (or use try/catch if async)
to network.disable() to log or handle errors instead of leaving the promise
unhandled; ensure these changes reference the existing stop(), isStarted,
start(), and network.disable() symbols and preserve the window.postMessage call.
| public errorWith(reason?: unknown): void { | ||
| if (reason instanceof Response) { | ||
| return this.respondWith(reason) | ||
| } | ||
|
|
||
| if (reason instanceof InternalError) { | ||
| this.#controller.errorWith(reason) | ||
| } | ||
|
|
||
| throw reason |
There was a problem hiding this comment.
Return after forwarding InternalError.
Line 150 already hands the failure to the interceptor. Falling through to Line 153 rethrows the same InternalError, so the "error" path can surface as an extra rejection.
Suggested fix
public errorWith(reason?: unknown): void {
if (reason instanceof Response) {
return this.respondWith(reason)
}
if (reason instanceof InternalError) {
- this.#controller.errorWith(reason)
+ return this.#controller.errorWith(reason)
}
throw reason
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public errorWith(reason?: unknown): void { | |
| if (reason instanceof Response) { | |
| return this.respondWith(reason) | |
| } | |
| if (reason instanceof InternalError) { | |
| this.#controller.errorWith(reason) | |
| } | |
| throw reason | |
| public errorWith(reason?: unknown): void { | |
| if (reason instanceof Response) { | |
| return this.respondWith(reason) | |
| } | |
| if (reason instanceof InternalError) { | |
| return this.#controller.errorWith(reason) | |
| } | |
| throw reason | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/experimental/sources/interceptor-source.ts` around lines 144 - 153,
The errorWith method incorrectly falls through after forwarding InternalError to
this.#controller.errorWith, causing the same InternalError to be rethrown;
update errorWith (in interceptor-source.ts) so that after calling
this.#controller.errorWith(reason) it immediately returns (similar to the
Response branch) to prevent the extra throw and double-rejection; make sure you
still handle Response via respondWith and only rethrow for other unknown
reasons.
| const hadError = await fetch('http://localhost/books', { | ||
| method: 'POST', | ||
| }).then( | ||
| () => expect.fail('Request must not succeed'), | ||
| () => true, | ||
| ) | ||
| expect(hadError).toBe(true) |
There was a problem hiding this comment.
Use GET /books here; POST never exercises the removed handler.
The original /books handler is registered with http.get(...), so this assertion passes even if resetHandlers(...) fails to replace it. Request the same method/path to actually prove the initial handler is gone.
🐛 Proposed fix
- const hadError = await fetch('http://localhost/books', {
- method: 'POST',
- }).then(
+ const hadError = await fetch('http://localhost/books').then(
() => expect.fail('Request must not succeed'),
() => true,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const hadError = await fetch('http://localhost/books', { | |
| method: 'POST', | |
| }).then( | |
| () => expect.fail('Request must not succeed'), | |
| () => true, | |
| ) | |
| expect(hadError).toBe(true) | |
| const hadError = await fetch('http://localhost/books').then( | |
| () => expect.fail('Request must not succeed'), | |
| () => true, | |
| ) | |
| expect(hadError).toBe(true) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/node/msw-api/setup-server/resetHandlers.node.test.ts` around lines 91 -
97, The test calls fetch('http://localhost/books', { method: 'POST' }) which
doesn't exercise the original handler registered with http.get, so change the
request to use GET (remove or set method to 'GET') so the fetch targets the same
method/path as the original handler; update the fetch invocation near the
hadError variable and keep the surrounding logic that expects the request to
fail after resetHandlers(...) to ensure the test actually verifies the original
GET /books handler was replaced.
setupRemoteServer#1617rettime#2558Changes
SetupApibehaves by introducing: network frames, sources, controllers, anddefineNetwork.instanceofcheck. Instead, thekindproperty of the handler is used.Usage
The introduces APIs are meant to be used from the explicit
msw/futureexport path. The path itself isn't documented as the APIs are considered experimental until the next major release.Roadmap
HandlersKindand just inlined them in the handler classes.__kindprivate field from handler classes and madekindpublic. This is a safe change as nobody should be using these fields.handlersand the constructor argument for the handlers controller seems odd. Choose just one?handlersoption. After all,handlersare only needed as the input to the controller.setupServer. ThedefineNetworkAPI is rather low-level. ExportNetowrkOptionsto the user can just override some of them?createSetupServerCommonApior its alternative. That's a crucial part of extending the default behaviors.SetuApiisn't that difficult to implement by hand.onUnhandledFramecouldn't affect the request resolution because it's called afterframe.resolve()is finished.onUnhandledRequest: 'error'to ensure that the request is errored (not bypassed). There's currently a bug sinceonUnhandledFrameis called afterframe.resolve(), which always callspassthrough()before the unhandled frame callback.WebSocketNetworkFrame, that won't happen (exception in one handler will short-circuit the entire loop).server.boundary()to establish ASL boundaries for Node.js tests. TheNetworkApimust remain the same across environments. Environment-specific APIs must come from the environment entrypoints (msw/node) and applied explicitly.AsyncHandlersController.boundary()instead of theboundary()method insetup-server.ts.readyStatetoNetworkApiso implementers don't have to introduce it themselves (e.g. removeisStartedfromsetup-worker.tsand use anetwork.readyStatecheck)..enable()leaking from the network sources. Not awaitingnetwork.disable()inserver.stop()feels weird.enable/disableindefineNetwork.SetupWorkerApiwas exported as a class before, now it's only a type. Re-implement that class similar to how we re-implementSetupServerApifor backwards compatibility.