Fix mocking of gRPC-web and Connect GET requests #100
Conversation
| // gRPC-web expects the body to be an AsyncIterable | ||
| if (contentType.startsWith("application/grpc-web")) { | ||
| body = createAsyncIterable([body]); | ||
| } |
There was a problem hiding this comment.
The body property of the type UniversalServerRequest is AsyncIterable<Uint8Array> | JsonValue. We're still passing in other types here. We do not need to special case the gRPC-web content type, only application/json.
Please update let body: any to not use any - this is how this bug slipped in in the first place. I suggest we use let body: UniversalServerRequest["body"].
Co-authored-by: Timo Stamm <[email protected]>
| @@ -184,18 +191,18 @@ async function universalHandlerToRouteResponse({ | |||
| }) { | |||
| const headers = await request.allHeaders(); | |||
| const abortSignal = new AbortController().signal; | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any -- The Serializable type isn't exposed by Playwright | |||
| let body: any; | |||
| let body: UniversalServerRequest["body"] = []; | |||
There was a problem hiding this comment.
Not sure of the best default value for this.
There was a problem hiding this comment.
[] is not a type error because the body can be pre-parsed JSON, and [] is a valid JSON value.
You are providing the request body [] to the call, if the original request had an empty body. That's no good. To provide an empty request body, create an empty byte stream with createAsyncIterable<Uint8Array>([]).
Let's also add a test for Connect GET, because I'm pretty sure we would crash on it.
|
@timostamm lmk what you think now. Was able to remove a lot of the eslint ignores also. |
|
@timostamm Round 3. Lmk what you think. |
| // Default body to an empty byte stream | ||
| let body: UniversalServerRequest["body"] = createAsyncIterable<Uint8Array>( | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Nit: Not crucial here, but you can simply declare the variable without assigning anything, as long as all code paths assign before use.
There was a problem hiding this comment.
Yeah, the only issue was there was a code path that didn't assign it (if buffer was null), but I fixed that so that I didn't initialize it. And now the AsyncIterable is only created just-in-time if needed.
| @@ -55,6 +62,17 @@ interface Options { | |||
| binaryOptions?: Partial<BinaryReadOptions & BinaryWriteOptions>; | |||
| } | |||
|
|
|||
| // Builds a regular expression for matching paths by appending the suffix onto | |||
| // base and escaping forward slashes and periods | |||
| baseURL, | ||
| baseURL + "?transport=connect", | ||
| baseURL + "?transport=connect&useHttpGet=true", | ||
| baseURL + "?transport=connect&format=binary", | ||
| baseURL + "?transport=connect&format=binary&useHttpGet=true", | ||
| baseURL + "?transport=grpcweb", | ||
| baseURL + "?transport=grpcweb&format=json", |
There was a problem hiding this comment.
Nice to have coverage against regressions 🎉
smaye81
changed the title
## What's Fixed * Fix playwright exports and update to current Connect deps (#101) by @smaye81 * Fix mocking of gRPC-web and Connect GET requests (#100) by @smaye81 **Full Changelog**: v0.3.1...v0.3.2
The
GrpcWebTransportexpects all request bodies to be anAsyncIterable, but Playwright's APIs for accessing the request body only return JSON or a Buffer. So, when creating a mock router, the handler should look at the content type and if it's gRPC-web, convert the body to anAsyncIterable.In addition, this fixes mocking GET requests via the Connect protocol.
Finally, this adds some tests to verify the mocks are correctly created for Connect and gRPC-web in both JSON and binary formats and with / without GET support for the Connect protocol.