Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: log ReadableStream responses as text/json #1622

Merged
merged 3 commits into from
Jun 8, 2023
Merged

fix: log ReadableStream responses as text/json #1622

merged 3 commits into from
Jun 8, 2023

Conversation

esquevin
Copy link
Contributor

This logs a promise giving you access to the content of the body in the logs

image

fix #1620

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 16, 2023 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 169311e:

Sandbox Source
MSW React Configuration

kettanaito
kettanaito reviewed Jun 8, 2023
View reviewed changes
src/utils/logging/prepareResponse.ts Outdated
)

if (parsedBody instanceof ReadableStream) {
const response = new Response(parsedBody, { headers: responseHeaders })
Copy link
Member

Choose a reason for hiding this comment

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

Alas, this won't work since prepareResponse is called in any environment, including Node. The current version of MSW still promises support for Node 16, which doesn't have the Fetch API available globally. This will break for those consumers.

We have to depend on something different, it seems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah :/ that's sad

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I just checked and this prepareResponse pipeline is only run in the browser. We should be safe relying on the Response constructor here, given we don't get any type errors in general.

Testing this locally now...

@kettanaito
Copy link
Member

kettanaito commented Jun 8, 2023 •
edited
Loading

I'm proposing to address this on a higher level, at the serializeResponse() function. That function always receives a response representation (not an actual Fetch API Response, that caught me) whose body is always a ReadableStream. So, let's always read it as text and then let the prepareResponse() use parseBody() that will, eventually, parse JSON response bodies into objects.

This way prepareResponse() is unchanged, no mutable state there either. prepareBody is unchanged too. The only tradeoff of this proposal is that serializeResponse becomes an async function, which is fine if we look at its consumption: only in the response:mocked life-cycle event listeners.

Oh, another upside for this is that the logged response now is plain text/Object, no Promises. They are tackled on the serializeResponse() level (thus the async/await)!

kettanaito
kettanaito reviewed Jun 8, 2023
View reviewed changes
src/utils/logging/serializeResponse.ts
// Serialize the response body to a string
// so it's easier to process further down the chain in "prepareResponse" (browser-only)
// and "parseBody" (ambiguous).
body: await response.text(),
Copy link
Member

Choose a reason for hiding this comment

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

Beware: Not an actual Response!

Using a normal Response here, we'd have to clone it first so every consumer down the chain would still be able to read it. But this is not a Fetch API Response instance here. It's our internal, custom thing since as per 1.2 we are still running in ambiguous, old environments without the global Fetch API.

Won't be a thing since 2.0.

Copy link
Member

Choose a reason for hiding this comment

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

Consequentially, this doesn't block the consumers from reading the response body again.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kettanaito after updating to 1.2.2 it looks like i'm getting errors here

Uncaught (in promise) TypeError: Failed to execute 'text' on 'Response': body stream already read
    at serializeResponse (index.js:1616:1)
    at Emitter.<anonymous> (index.js:1660:1)
    at Emitter.onceListener (Emitter.js:40:1)
    at Emitter.js:75:1
    at Array.forEach (<anonymous>)
    at Emitter.emit (Emitter.js:74:1)
    at source.emit (index.js:2036:1)
    at index.js:1756:1
    at ServiceWorkerContainer.<anonymous> (index.js:2140:1)
serializeResponse @ index.js:1616
(anonymous) @ index.js:1660
onceListener @ Emitter.js:40
(anonymous) @ Emitter.js:75
emit @ Emitter.js:74
source.emit @ index.js:2036
(anonymous) @ index.js:1756
(anonymous) @ index.js:2140
await in (anonymous) (async)
onceListener @ Emitter.js:40
(anonymous) @ Emitter.js:75
emit @ Emitter.js:74
source.emit @ index.js:2036
(anonymous) @ index.js:1756
(anonymous) @ index.js:2140

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that quiet: true gets rid of those issues though, since it no longer tries to read that response twice?

Copy link
Member

Choose a reason for hiding this comment

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

@mattcosta7, do you have a reproduction case for this?

I was concerned about this too but the response we use as of 1.x is not a Fetch AIP Response. It doesn't even have the .clone() method and is meant purely for the internal representation of responses. Thus, there's no issue of reading the body multiple times.

I'd like to look closer at this.

Copy link
Contributor

@mattcosta7 mattcosta7 Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll try to get something together when I can. it seems to happen with a pretty minimal case (in the work app i'm looking at now)

rest.post('<url>', async (req, res, ctx) => {
   return res(ctx.status(200))
})

that alone caused this in the work project I'm looking at now

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting - haven't managed a repro on this, despite the 'handler' being super thin. feel free to disregard and i'll keep trying to understand how/where it's failing.

in the meantime, quiet avoids the issue for us and keeps the console a bit clearer so that's ok

Copy link

Choose a reason for hiding this comment

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

Hey guys! Hopping in the chat because I'm having the same issue with v1.2.2:

👀 Console error 
index.js:1402 [MSW] 07:22:18 query AuthUser (200 OK)
index.js:1616 Uncaught (in promise) TypeError: Failed to execute 'text' on 'Response': body stream already read
    at serializeResponse (webpack-internal:///(app-client)/../../node_modules/.pnpm/msw@1.2.2_typescript@4.8.4/node_modules/msw/lib/index.js:1616:27)
    at Emitter.eval (webpack-internal:///(app-client)/../../node_modules/.pnpm/msw@1.2.2_typescript@4.8.4/node_modules/msw/lib/index.js:1660:25)
    at Emitter.onceListener (webpack-internal:///(app-client)/../../node_modules/.pnpm/strict-event-emitter@0.4.6/node_modules/strict-event-emitter/lib/Emitter.js:40:22)
    at eval (webpack-internal:///(app-client)/../../node_modules/.pnpm/strict-event-emitter@0.4.6/node_modules/strict-event-emitter/lib/Emitter.js:75:22)
    at Array.forEach (<anonymous>)
    at Emitter.emit (webpack-internal:///(app-client)/../../node_modules/.pnpm/strict-event-emitter@0.4.6/node_modules/strict-event-emitter/lib/Emitter.js:74:19)
    at source.emit (webpack-internal:///(app-client)/../../node_modules/.pnpm/msw@1.2.2_typescript@4.8.4/node_modules/msw/lib/index.js:2036:20)
    at eval (webpack-internal:///(app-client)/../../node_modules/.pnpm/msw@1.2.2_typescript@4.8.4/node_modules/msw/lib/index.js:1756:23)
    at ServiceWorkerContainer.eval (webpack-internal:///(app-client)/../../node_modules/.pnpm/msw@1.2.2_typescript@4.8.4/node_modules/msw/lib/index.js:2140:17)
serializeResponse @ index.js:1616
eval @ index.js:1660
onceListener @ Emitter.js:40
eval @ Emitter.js:75
emit @ Emitter.js:74
source.emit @ index.js:2036
eval @ index.js:1756
eval @ index.js:2140
await in eval (async)
onceListener @ Emitter.js:40
eval @ Emitter.js:75
emit @ Emitter.js:74
source.emit @ index.js:2036
eval @ index.js:1756
eval @ index.js:2140

The following solutions worked for me:

  1. use option quiet: true
  2. downgrade to v1.2.1

With v1.2.2 and quiet set to false, the console error occurs on the second request to the same endpoint. In our case we are making two requests to an AuthUser query in GraphQL. The first request is logged to the console and the second request-log is followed by the error. That might be why the error says "body stream already read" — I hope that's helpful!

I'm on a work repo so I can't provide a reproduction env easily right now. That said, our implementation isn't special. I am seeing the console error while mocking responses (in local development) to our GraphQL API within a Next.js app.

@kettanaito kettanaito changed the title fix: log human readable response fix: log ReadableStream responses as text/json Jun 8, 2023
@esquevin
Copy link
Contributor Author

esquevin commented Jun 8, 2023 •
edited
Loading

I'm proposing to address this on a higher level, at the serializeResponse() function. That function always receives a response representation (not an actual Fetch API Response, that caught me) whose body is always a ReadableStream. So, let's always read it as text and then let the prepareResponse() use parseBody() that will, eventually, parse JSON response bodies into objects.

This way prepareResponse() is unchanged, no mutable state there either. prepareBody is unchanged too. The only tradeoff of this proposal is that serializeResponse becomes an async function, which is fine if we look at its consumption: only in the response:mocked life-cycle event listeners.

Oh, another upside for this is that the logged response now is plain text/Object, no Promises. They are tackled on the serializeResponse() level (thus the async/await)!

Yeah I tried something with that PR but I don't know / understand MSW codebase well enough to do something appropriate. This PR should probably be closed in favor of something else.

@kettanaito
Copy link
Member

No need to close this, I pushed the proposed changes already. They still address the issue so this pull request feels like an appropriate place. Waiting for the CI to pass.

@kettanaito
Copy link
Member

Let's go! 🚀 Thank you for helping me fix this, @esquevin! Welcome to contributors.

@kettanaito kettanaito merged commit 4c7e952 into mswjs:main Jun 8, 2023
@esquevin
Copy link
Contributor Author

esquevin commented Jun 8, 2023

Thx to you ! MSW is truly awesome to use

@kettanaito kettanaito mentioned this pull request Jun 23, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattcosta7 mattcosta7 mattcosta7 left review comments

@kettanaito kettanaito kettanaito left review comments

@strativd strativd strativd left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Log response body in plain text/json instead of a ReadableStream
4 participants
@esquevin @kettanaito @mattcosta7 @strativd