fix: batching with zod .optional() input

[KATT]

• JSON.stringify([undefined]) === [null]
• this causes issues with .optional() zod schemas as it expects an object or undefined, not null
• fixed by ugly hack that runs validator twice when it fails if the raw input was === null.
  fixed by @simonedelmann's idea

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

www – ./www

🔍 Inspect: https://vercel.com/trpc/www/EuKCQujsNkMFUEYtU7NtUU38uayD
✅ Preview: https://www-git-feature-batching-edge-case.tmp.trpc.io

todomvc – ./examples/next-prisma-todomvc

🔍 Inspect: https://vercel.com/trpc/todomvc/5EDB1nCkpzGnJJHsyj6bdJdohQaz
✅ Preview: https://todomvc-git-feature-batching-edge-case.tmp.trpc.io

[simonedelmann]

I've just checked the PR. I am not sure if I like the hack, but it should do the job. But...

I am not sure if we should merge this. If we expect the library to always parse values correctly, we should set a transformer (SuperJSON or something similar) by default and not just handle one specific edge case. But if the library by default uses JSON.stringify and JSON.parse for encoding/decoding values and we expect our users to deal with its issues anyway (sending Dates for example), they should also be aware of this edge case and manage it on their own.

[KATT]

I am not sure if we should merge this. If we expect the library to always parse values correctly, we should set a transformer (SuperJSON or something similar) by default and not just handle one specific edge case, [...] , they should also be aware of this edge case and manage it on their own.

I understand your point of view, I just think this is a very common edge-case, especially since I want to enable batching per default (#160).

I don't wanna add transformers as default as they add a perf hit and everyone don't benefit from having them. At least not superjson, as it's slow.

I wish JSON.parse() would throw on dates/map/set as default - I might actually add a specific callback to JSON.parse() to make this happen with a note that they should add a transformer as a follow-up.

[simonedelmann]

I understand your point of view, I just think this is a very common edge-case, especially since I want to enable batching per default (#160).

I see. So it is probably worth handling this specific case. Then I have only one more concern: Are null and undefined the only two values which are represented by "null" in JSON? So we make sure not to parse anything as undefined which actually didn't was undefined before.

I wish JSON.parse() would throw on dates/map/set as default - I might actually add a specific callback to JSON.parse() to make this happen with a note that they should add a transformer as a follow-up.

I like the idea of throwing on Dates/Maps/Sets/... by default. The only thing I would suggest then is to do a little research, what kind of problems JSON.stringify/parse has, so we are not only addressing a few but all (or at least many) of them.

[simonedelmann]

One more question: Couldn't we solve this client-side? So if the input value is null, we send JSON.stringify(null). But if input value is not existing at all, we just send an empty string "", which is no valid JSON then but is clearly identifiable as undefined?

[KATT]

One more question: Couldn't we solve this client-side? So if the input value is null, we send JSON.stringify(null). But if input value is not existing at all, we just send an empty string "", which is no valid JSON then but is clearly identifiable as undefined?

Good idea, but it would break this code instead

export const appRouter = createRouter()
  .query('hello', {
    input: z.string().min(1).optional(),
    resolve: ({ input, ctx }) => {
      return `hello ${input ?? 'world'}`;
    },
  })

// [...]

await client.query('hello'); // will throw 

[simonedelmann]

But why? .optional() does allow a value of undefined. So if we have no input client-side, why can't we call the parser with parser(undefined)?

[KATT]

@all-contributors add @simonedelmann for reviews

[allcontributors]

@KATT

I've put up a pull request to add @simonedelmann! 🎉