feat!: rewrite package to new network source architecture#48
feat!: rewrite package to new network source architecture#48christoph-fricke wants to merge 20 commits intomswjs:mainfrom
Conversation
There was a problem hiding this comment.
@kettanaito I am struggling a bit with implementing WebSocket handling, and would like to align on an approach before continuing. I don't have experience with using WebSocket mocking in MSW, which makes this a bit harder as well. So far I thought about three approaches to tackling this.
1. PlaywrightWebSocketNetworkFrame extends WebSocketNetworkFrame
Similar to the PlaywrightHttpNetworkFrame implementation, this seems like the most obvious choice. When constructing an implementation of the frame, it requires a WebSocketClientConnection and WebSocketServerConnection connection. First idea: Let's use the already existing Playwright connection classes that implement the WebSocket(Client|Server)ConnectionProtocol.
const frame = new PlaywrightWebSocketNetworkFrame({
connection: {
client: new PlaywrightWebSocketClientConnection(route),
server: new PlaywrightWebSocketServerConnection(route),
info: { protocols: [] },
},
})This does not work! It results in type errors for both client and server:
- client: Type 'PlaywrightWebSocketClientConnection' is missing the following properties from type 'WebSocketClientConnection': socket, transport, [kEmitter$1]
- server: Type 'PlaywrightWebSocketServerConnection' is missing the following properties from type 'WebSocketServerConnection': client, transport, createConnection, mockCloseController, and 7 more.
This also explains the TS errors in the existing fixture after updating MSW when calling webSockerHandler.run(...), and might explain why all WebSocket related tests have been failing.
=> Takeaway: Implementing WebSocket(Client|Server)ConnectionProtocol is not enough. We have to use WebSocket(Client|Server)Connection if we want to extend WebSocketNetworkFrame.
2. Implementing a WebSocketTransport
Since WebSocket(Client|Server)Connection are already usable classes, it might be enough to implement a custom WebSocketTransport, and construct WebSocket(Client|Server)Connection directly with a PlaywrightWebSocketTransport.
My concern with this approach is that this might not work. The two classes appear to be a bit too closely related to an actual WebSocket. Both require a WebSocket instance for construction and do operations with it to some extend. I fear that we cannot provide a reliable WebSocket instance (mock) with the stuff we get from Playwright's WebSocketRoute.
3. Extend a "low-level" NetworkFrame<"ws">
Maybe we do not need to extend WebSocketNetworkFrame. All we have to do is resolve/proxy WebSocket messages using the WebSocketRoute (famous last words...). Maybe the simplest, most robust approach, is a PlaywrightWebSocketNetworkFrame that extends NetworkFrame<"ws"> directly, and uses WebSocketRoute instead of WebSocket(Client|Server)Connection.
I started this third approach in this file. But as you can see, msw/experimental is missing a bunch of exports to be able to directly extend NetworkFrame. So I am wondering if this approach should be avoided at all cost.
What are your thoughts on this? What do you think is the best approach for handling Playwright WebSocket routes with the new network source architecture? Are there missing pieces in MSW to make this feasible?
There was a problem hiding this comment.
Your first approach sounds correct. The type mismatch might as well be a bug. WebSocketClientConnectionProtocol type is a general type I created exactly for this reason (so the implementers don't have to worry about transport).
Yeah, it's definitely a bug. In websocket-frame.ts:
export interface WebSocketNetworkFrameOptions {
connection: WebSocketConnectionData
}I'm getting the WebSocketConnectionData from the Interceptors as-is, but that's a mistake. That type is a narrower type than what the frame should care about.
We should probably annotate that connection option as WebSocketHandlerConnection from WebSocketHandler.ts, which contains the broader WebSocketClientConnectionProtocol.
Can you open an issue for this in the MSW repo, please? 🙏 I think this is a great find.
There was a problem hiding this comment.
@kettanaito The rewrite is complete so far. All tests pass (when the "type as value" bug is fixed in MSW or locally) and the new implementation feels quite solid already. I think next good steps are:
- We give the implementation a proper review
- Clarify open questions and resolve missing MSW parts
- Remove the old
fixture.tsimplementation - Update the README and add some JSDoc comments to the source class and its options.
| import type { RequestHandler } from 'msw' | ||
| import type { NetworkFrameResolutionContext } from '../../node_modules/msw/lib/core/experimental/frames/network-frame.mjs' | ||
| import type { UnhandledFrameHandle } from '../../node_modules/msw/lib/core/experimental/on-unhandled-frame.mjs' |
There was a problem hiding this comment.
❌ MSW should export these types before we merge this.
| import type { WebSocketHandler } from 'msw' | ||
| import type { NetworkFrameResolutionContext } from '../../node_modules/msw/lib/core/experimental/frames/network-frame.mjs' | ||
| import type { UnhandledFrameHandle } from '../../node_modules/msw/lib/core/experimental/on-unhandled-frame.mjs' |
There was a problem hiding this comment.
❌ MSW should export these types before we merge this.
| } | ||
|
|
||
| passthrough(): void { | ||
| this.#route.connectToServer() |
There was a problem hiding this comment.
💭 Not sure if this approach, or calling this.data.connection.server.connect() is better? The latter registers pending event listeners from the mock. Does it even matter in the cases where passthrough is called?
| } | ||
| } | ||
|
|
||
| class PlaywrightWebSocketClientConnection implements WebSocketClientConnectionProtocol { |
There was a problem hiding this comment.
💡 PlaywrightWebSocketClientConnection and PlaywrightWebSocketServerConnection are pretty much copied. Only made slight adjustment such as aligning the #route field naming.
| } from './route-utils.js' | ||
|
|
||
| export interface PlaywrightSourceOptions { | ||
| skipAssetRequests?: boolean |
There was a problem hiding this comment.
💭 With support for custom route patterns, skipAssetRequests might be obsolete.
I added support for custom patterns, because in my experience most mock setups mock API(s) behind one (a few at most) endpoints. Alongside registering multiple PlaywrightSource sources, custom patterns can help reduce the interception overhead for this common case. What do you think about this?
There was a problem hiding this comment.
Yes, if we go with the route of mapping handlers to page.route(), then I suppose there's no need in skipAssetRequests anymore. Just need to make sure the tests pass.
| interface Fixtures { | ||
| handlers: Array<AnyHandler> | ||
| network: NetworkFixture | ||
| network: ReturnType<typeof defineNetwork<PlaywrightSource[]>> |
There was a problem hiding this comment.
❌ MSW is missing a export for typing NetworkApi. Ideally, the type does not have to declare all sources, because a "network consumer" that uses the type does not care about the internals (interception sources) of the given network.
Warning: Skipping the sources generic does not seem to work. Specific networks cannot be passed to it: 'NetworkApi<PlaywrightSource[]>' is not assignable to parameter of type 'NetworkApi<NetworkSource<any>[]>.
I can also imagine a NetworkApi<EventMap> to be very useful. When I define a function that accepts a network, I do not care about the underlying sources. I can't access them anyway. However, I can access events dispatched by the network frames. So I would find it more useful to be able to declare "I need a network that at least dispatches 'request:start' events", i.e. NetworkApi<{"request:start": RequestEvent}>.
There was a problem hiding this comment.
The network-source contract is pretty important though. You cannot declare a network without caring for its sources as they dictate the interception and also affect what enable/disable methods do.
EventMap, on the other hand, is purely source-specific and has nothing to do with the network. Think of the network as a shell that manages sources + routes their frames through handlers. It does nothing more. The sources decide which frames to yield and which handlers to match, the handlers decide how to handle given network messages.
|
|
||
| await network.enable() | ||
| await use(network) | ||
| if (network.readyState === 1) { |
There was a problem hiding this comment.
❌ MSW lacks an export for the NetworkReadyState enum. Alternatively, this type should string literal based for better readability.
| context, | ||
| const network = defineNetwork({ | ||
| sources: [new PlaywrightSource(context)], | ||
| onUnhandledFrame: 'bypass', |
There was a problem hiding this comment.
💡 The existing fixture.ts implementation declared onUnhandledRequest: "bypass". The network API sets the default to "warning", which creates a lot of noise in the tests coming from the initial page requests.
The new implementation has no way to differentiate between default set by MSW and value chosen by the user. So we cannot fully recreate the old behavior here.
| expect.soft(consoleSpy.callCount).toBe(2) | ||
| expect(consoleSpy.getCall(1)?.args).toEqual([ | ||
| expect.soft(consoleSpy.callCount).toBe(3) | ||
| expect(consoleSpy.getCall(2)?.args).toEqual([ |
There was a problem hiding this comment.
💭 The implementation in fixture.ts never tried to resolve Vite's dev-server WebSocket here, because it aborts early when no WebSocket handlers are defined. I let the orchestration happen completely in WebSocketNetworkFrame.resolve(), which leads to the additional warning.
If needed, we could add an early passthrough in PlaywrightWebSocketNetworkFrame to avoid the additional WebSocket warning.
| connection: { | ||
| client: new PlaywrightWebSocketClientConnection(options.route), | ||
| server: new PlaywrightWebSocketServerConnection(options.route), | ||
| info: { protocols: [] }, |
There was a problem hiding this comment.
❌ Depends on work in mswjs/msw#2710. Currently, this code surfaces a TS error.
Although, it (surprisingly) works at runtime already. At least all tests pass.
This PR rewrites
@msw/playwrightto be based on the new network source architecture released in MSW v2.13.0.Todo
PlaywrightSourcethat extendsNetworkSource.BrowserContextorPageas an interception targetPlaywrightHttpNetworkFrame extends HttpNetworkFrame.PlaywrightHttpNetworkFrameintoPlaywrightSource.PlaywrightWebSocketNetworkFrame extends WebSocketNetworkFrame.PlaywrightHttpNetworkFrameintoPlaywrightSource.quitas the default inPlaywrightHttpNetworkFramequitas the default inPlaywrightWebSocketpNetworkFrame"bypass"."warn". We cannot differentiate between user choice and MSW default.fixture.tscodeQuestions / Decisions for the Implementation
setup***in MSW) as a temporary wrapper? Or go all-in into only exposing a network source?PlaywrightSourcewhich accepts either aBrowserContextorPage.Additional topics in MSW's network source architecture
msw/experimentalcodeverbatimModuleSyntax.verbatimModuleSyntaxconsistently in exports msw#2702onUnhandledFrameonUnhandledFramereceive aframe. This appears to always be types asAnyNetworkFrame.HttpNetworkFrame?if (frame instanceof HttpNetworkFrame)the intended approach?instanceofnarrowing check in the MSW source code. Looks like the intended approach.HttpNetworkFrameOptionsandWebSocketNetworkFrameOptionsHttpNetworkFrameOptionsis not exported. Thus cannot be used to extend options for a frame sub-class.HttpNetworkFramesub classes.HttpNetworkFrameorWebSocketNetworkFrame. Directly extendingNetworkFramedoes not look advisable.reason instanceof Responsecheck in HTTP framesInterceptorHttpNetworkFrameandServiceWorkerHttpNetworkFrameerrorWithimplementations have areason instanceof Responsecheck.