fix(server): fix regression introduced by #5017 #5039

jussisaurio · 2023-11-14T20:25:58Z

🎯 Changes

There were two bug reports about 10.43.3, the cause of which can be traced back to PR #5017 (which fixed another inference bug... seeing a pattern here)

I think in both bug reports the cause has something to do with using generics to construct either tRPC instances or tRPC middlewares using a type parameter that is not constrained to object, which causes TS to infer incompatible types for middleware builders after Overwrite was changed to only do the key-wise merge on types that extend object.

This PR basically reverts the changes in #5017 and replaces it with a check to see whether TWith (the type that overwrites the target type) extends a JS primitive, and if so, wholly replaces it. This achieves the intended end result of #5017 while (hopefully) fixing the issues introduced in 10.43.3 and reported in #5037.

This PR includes a test with which I was able to reproduce a compile error similar to the one(s) reported in #5037, and which was fixed with the added changes.

✅ Checklist

I have followed the steps listed in the Contributing guide.
If necessary, I have added documentation related to the changes made.
I have added or updated the tests related to the changes made.

vercel · 2023-11-14T21:05:19Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
next-prisma-starter	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 17, 2023 11:21am
og-image	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 17, 2023 11:21am
trpc-next-app-dir	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 17, 2023 11:21am

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
www	⬜️ Ignored (Inspect)	Visit Preview		Nov 17, 2023 11:21am

jussisaurio · 2023-11-15T17:40:10Z

Alternative simpler implementation that also works (works i.e. does not break tests):

/**
 * @internal
 * Overwrite properties in `TType` with properties in `TWith`
 * Only overwrites properties when the type to be overwritten
 * is an object. Otherwise it will just use the type from `TWith`.
 */
export type Overwrite<TType, TWith> = TWith extends any
  ? TType extends object
    ? {
        [K in keyof TType | keyof TWith]: K extends keyof TWith
          ? TWith[K]
          : K extends keyof TType
          ? TType[K]
          : never;
      }
    : TWith
  : never;

I am not exactly sure why this is 🤷

KATT · 2023-11-15T20:23:19Z

cc @jussisaurio could you have a look at the conflicts? 🙏

jussisaurio · 2023-11-15T21:14:42Z

Feel a bit bad about these last few bugs (the one with input(z.string() getting mangled, plus #5037)... they were basically caused by fixing other bugs involving experimental_standaloneMiddleware which is an experimental feature. So apologies for the churn, hopefully the treadmill stops pretty soon.

As a general note, I find the current propagation of types in tRPC's middlewares really, really hard to reason about (and, consequently, to make changes to it) and I think @Nick-Lucas probably feels the same way as well. Wondering if for a future major version it might make sense to think about a full rewrite of the middleware system?

Nick-Lucas · 2023-11-15T22:07:06Z

I think @Nick-Lucas probably feels the same way as well. Wondering if for a future major version it might make sense to think about a full rewrite of the middleware system?

Yeah so I wrote this earlier in the year: https://github.com/trpc/trpc/pull/4239/files I then abandoned it because I was trying to handle all the corner cases and it just was not working out, for identical reasons to our t.middleware and standalone middleware causing problems

I ran into all sorts of problems with the chainability of two declared types. The problem is typescript's type system isn't really built for combining static definitions like A->B + A->C = A->C as those two aren't assignable to each other, and tricking those consistently into believing the formula is A->B + B->C = A->C is what we have to achieve to compile. So the fix I provided in the Issue by using a generic typed function is the most sound way to solve the problem because generics are designed to solve that chainability problem, if that makes sense? Anyway, Sentry likely weren't a cause, they just got caught up in the bad chaining.

So I don't think a re-write of middlewares will be a solution, as part of my problem implementing this was the complexity of types, and we might be able to have a better crack at this in v11 with the simplification work that's been done. But otherwise I suggest we promote the factory/higher-order-function approach instead of creating an API to generate a middleware, because I've found that behaves much more consistently

jussisaurio · 2023-11-16T14:56:51Z

packages/tests/server/regression/issue-5034-input-with-index-signature.test.ts

+    expectTypeOf<Input>().toEqualTypeOf<{ name: string }>();
+    expectTypeOf<Output>().toEqualTypeOf<{
+      name: string;
+      [symbol]: true;


Having a hard time with this... why are we expecting inferred outputs to contain symbols (which is not JSON serializable)? Plus, the input (which the query returns) doesn't contain the symbol, ctx does. And if you look at ctx inside the query, it's inferred correctly

I messed up the test, it actually works as intended, sorry :D

jussisaurio requested a review from a team as a code owner November 14, 2023 20:25

github-actions bot added the @trpc/server label Nov 14, 2023

jussisaurio mentioned this pull request Nov 14, 2023

fix(client+server): avoid losing type information w/ index signatures #5035

Merged

3 tasks

vercel bot deployed to Preview – trpc-next-app-dir November 14, 2023 21:05 View deployment

vercel bot deployed to Preview – next-prisma-starter November 14, 2023 21:06 View deployment

vercel bot deployed to Preview – og-image November 14, 2023 21:06 View deployment

vercel bot deployed to Preview – todomvc November 14, 2023 21:06 View deployment

vercel bot deployed to Preview – trpc-next-app-dir November 14, 2023 21:07 View deployment

vercel bot deployed to Preview – next-prisma-starter November 14, 2023 21:07 View deployment

vercel bot deployed to Preview – og-image November 14, 2023 21:07 View deployment

vercel bot deployed to Preview – todomvc November 14, 2023 21:07 View deployment

jussisaurio mentioned this pull request Nov 14, 2023

v7.80.0 breaks tRPC middleware handler (Typescript) getsentry/sentry-javascript#9536

Closed

3 tasks

KATT previously approved these changes Nov 15, 2023

View reviewed changes

jussisaurio added 3 commits November 15, 2023 23:02

fix(server): fix regression introduced by #5017

c63604e

Rename test description

c57097c

fix lint

eec398d

jussisaurio dismissed KATT’s stale review via eec398d November 15, 2023 21:03

jussisaurio force-pushed the fix-5037 branch from f3e303b to eec398d Compare November 15, 2023 21:03

vercel bot deployed to Preview – next-prisma-starter November 15, 2023 21:03 View deployment

vercel bot deployed to Preview – og-image November 15, 2023 21:03 View deployment

vercel bot deployed to Preview – todomvc November 15, 2023 21:04 View deployment

vercel bot deployed to Preview – trpc-next-app-dir November 15, 2023 21:05 View deployment

vercel bot deployed to Preview – www November 15, 2023 21:14 View deployment

found something not working

e603b48

vercel bot deployed to Preview – trpc-next-app-dir November 16, 2023 09:11 View deployment

vercel bot deployed to Preview – og-image November 16, 2023 09:11 View deployment

vercel bot deployed to Preview – next-prisma-starter November 16, 2023 09:11 View deployment

vercel bot deployed to Preview – todomvc November 16, 2023 09:11 View deployment

jussisaurio commented Nov 16, 2023

View reviewed changes

KATT added 4 commits November 17, 2023 12:08

fixes and improvements

e359fec

watch tests

e79b1f4

skipLibCheck

394be05

revert stuff

a2a14f0

vercel bot deployed to Preview – trpc-next-app-dir November 17, 2023 11:21 View deployment

vercel bot deployed to Preview – next-prisma-starter November 17, 2023 11:21 View deployment

vercel bot deployed to Preview – og-image November 17, 2023 11:21 View deployment

vercel bot deployed to Preview – todomvc November 17, 2023 11:22 View deployment

KATT approved these changes Nov 17, 2023

View reviewed changes

KATT merged commit de85898 into main Nov 17, 2023
36 of 37 checks passed

KATT deleted the fix-5037 branch November 17, 2023 11:37

github-actions bot locked as resolved and limited conversation to collaborators Dec 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(server): fix regression introduced by #5017 #5039

fix(server): fix regression introduced by #5017 #5039

jussisaurio commented Nov 14, 2023

vercel bot commented Nov 14, 2023 •

edited

Loading

jussisaurio commented Nov 15, 2023

KATT commented Nov 15, 2023

jussisaurio commented Nov 15, 2023

Nick-Lucas commented Nov 15, 2023

jussisaurio Nov 16, 2023

KATT Nov 17, 2023

fix(server): fix regression introduced by #5017 #5039

fix(server): fix regression introduced by #5017 #5039

Conversation

jussisaurio commented Nov 14, 2023

🎯 Changes

✅ Checklist

vercel bot commented Nov 14, 2023 • edited Loading

jussisaurio commented Nov 15, 2023

KATT commented Nov 15, 2023

jussisaurio commented Nov 15, 2023

Nick-Lucas commented Nov 15, 2023

jussisaurio Nov 16, 2023

Choose a reason for hiding this comment

KATT Nov 17, 2023

Choose a reason for hiding this comment

vercel bot commented Nov 14, 2023 •

edited

Loading