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.

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

tRPC mutations hang with Koa adapter (solved; due to bodyParser middleware conflict) #24

Closed
aseemk opened this issue Mar 3, 2024 · 2 comments · Fixed by #26
Closed
Assignees

Comments

@aseemk
Copy link

aseemk commented Mar 3, 2024

Hi there. Thanks for this library! I hit an issue using it, where queries worked, but mutations would hang indefinitely, and never actually execute my procedure/function.

I managed to figure this issue out, but I thought I'd document it here for anyone else that runs into it (since I spent more than an hour debugging it).

The cause was that:

  • Mutation inputs come through the request body (whereas query string inputs come through the query string)

  • tRPC's Node adapter (which this library wraps) thus attempts to read/stream the request body

  • I was already using Koa's @koa/bodyparser middleware for other uses in my app. And that middleware already reads/streams the request body itself, so tRPC was indefinitely waiting around for data and end events that never came.

Fortunately:

  • tRPC's Node adapter looks for a pre-populated body property on the native Node request if one is set.

  • @koa/bodyparser provides a patchNode option to add this property to the native Node request (in addition to the Koa context's request).

So the solution is: if you're using @koa/bodyparser, set patchNode: true for tRPC to work with Koa!

  app.use(
    bodyParser({
      // This is required for tRPC to work! Since this `bodyParser` middleware reads & drains the
      // request stream, while tRPC tries to stream the body too by default, which then hangs forever.
      // The only way tRPC short-circuits that if a `body` is already populated on the native Node
      // request object, which this `patchNode` option does.
      patchNode: true,
      // `encoding` needs to be explicitly specified for the TypeScript type (likely wrong).
      encoding: "utf-8",
    })
  );

One last note: we were actually using koa-bodyparser — a subtly different package name that stopped getting updates after v4 — while the patchNode option was only added in v5! So fixing this also required changing & upgrading from koa-bodyparser to @koa/bodyparser.

Consider adding a note about this to the readme. Even though this isn't directly related to this Koa tRPC adapter, it might not be uncommon for Koa users to be using the bodyparser middleware too.

Thanks and hope this helps others!

@BlairCurrey
Copy link
Owner

BlairCurrey commented Mar 10, 2024

Hi @aseemk. Thank you for this detailed writeup, very helpful. I see the issue as you describe. I am doing a little more research but will definitely incorporate your feedback here to make it easier for others.

Those requiring the old koa-bodyparser may be able to utilize use its disableBodyParser option. Probably in conjunction with createKoaMiddleware's prefix. Something like this is working with mutations for me:

const prefix = "/trpc";

const app = new Koa();

app.use(async (ctx, next) => {
  if (ctx.path.startsWith(prefix)) ctx.disableBodyParser = true;
  await next();
});

app.use(bodyParser());

app.use(
  createKoaMiddleware({
    router: appRouter,
    prefix,
  })
);

FWIW this should work with @koa/bodyparser as well.

If it happens that you can add the bodyparser middleware after this createKoaMiddleware then that would avoid this problem as well.

Also wondering if there is anything more we can do in this library to handle this (or fail more gracefully) such as a looking at the raw body, but not sure if that ultimately makes sense.

EDIT: Actually, I think the disableBodyParser workaround is effectively equivalent to adding createKoaMiddleware before the body parser middleware because the body will not be parsed and available in koa prior to reaching the tRPC router.

BlairCurrey added a commit that referenced this issue Mar 14, 2024
- fixes body parsing issue raised here: #24
- data stream gets consumed by body parser
- changed middleware to look for parsed body and make available where trpc expects it
@BlairCurrey
Copy link
Owner

BlairCurrey commented Mar 14, 2024

I made a fix that solves it across the board (confirmed with koa-bodyparser and @koa/bodyparser at least). patchNode with @koa/bodyparser will still work but wont be necessary.

I added a check for the parsed body on ctx.request and add it to the node request if found. I imagine that covers the vast majority of scenarios but I also added a note in the readme briefly stating the expection for parsed bodies to found on the ctx.request just in case.

EDIT: This definitely seems like the way to go. I played around with the trpc express adapter and tried to produce the same error. Parsing the body with express.json() (body-parser under the hood) before adding the adapter middleware does not produce this issue because body-parser puts the body on the req (which we are now doing here as well).

BlairCurrey added a commit that referenced this issue Mar 14, 2024
* chore: update packageManager declaration

- lockfile produced with more recent pnpm and version in package.json was fairly old

* docs: update readme with koa bodyparser usage example

* chore: fix typos, change example order, other minor edits

* chore: more minor readme edits

* fix: mutation hang when data stream consumed by parser

- fixes body parsing issue raised here: #24
- data stream gets consumed by body parser
- changed middleware to look for parsed body and make available where trpc expects it

* test: refactor and add bodyparser integration cases

* chore: rm unecessary ts options

- recently added but ultimately determined they arent required. one of them causes the build to fail

* docs: mention/link gh issue in body parser note
@BlairCurrey BlairCurrey self-assigned this Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants