icflorescu / trpc-sveltekit-example

A sample SvelteKit application built to illustrate the usage of ✨ trpc-sveltekit.
https://icflorescu.github.io/trpc-sveltekit
ISC License
135 stars 19 forks source link

First request running twice #4

Closed thenbe closed 2 years ago

thenbe commented 2 years ago

Right now, visiting a page that has a trpc query in it's load function (ex. /authors) , will result in that trpc query running twice. First on the server, then again on the client. This is happening (I think) because we're not using the fetch provided by load.

Note: This is only applicable to the first request, since all other requests after that are client side only.

There two ways to go around this issue:

  1. Move the trpc query from load to a page endpoint.
  2. Find a way to make trpc use the fetch provided by load. No idea how.

This would ensure that the request would not happen twice. It would happen only on the server.

From the docs:

When fetch runs on the server, the resulting response will be serialized and inlined into the rendered HTML. This allows the subsequent client-side load to access identical data immediately without an additional network request.

thenbe commented 2 years ago

Update: Moving the trpc query from load to a page endpoint breaks the Date type. I tried passing load's fetch to trpc client but couldn't really get it to click.

icflorescu commented 2 years ago

Thanks a lot for investigating this and coming up with the PR!

Update: Moving the trpc query from load to a page endpoint breaks the Date type. I tried passing load's fetch to trpc client but couldn't really get it to click.

Is this happening even if you're using the superjson transformer?

icflorescu commented 2 years ago

Also, there might be a slight problem with this:

const url = browser ? '/trpc' : 'http://localhost:3000/trpc';
const client = (loadFetch?: typeof fetch) =>
  trpc.createTRPCClient<Router>({
    url: loadFetch ? '/trpc' : url,
      ...(loadFetch && { fetch: loadFetch }),
});

The client gets recreated every time. I don't how what's the overhead, but I think we could better do it like this:

let client: TRPCClient<Router>;
export default (loadFetch?: typeof fetch) => {
  if (!client) {
    const url = browser ? '/trpc' : 'http://localhost:3000/trpc';
    client = trpc.createTRPCClient<Router>({
      url: loadFetch ? '/trpc' : url,
      transformer: trpcTransformer,
      ...(loadFetch && { fetch: loadFetch })
    });
  }
  return client;
};

What do you think?

thenbe commented 2 years ago

Is this happening even if you're using the superjson transformer?

Yeah.

The client gets recreated every time. I don't how what's the overhead.

That's true. It'd be great if there was a way to avoid that. Besides the fact that there would be multiple clients floating around, one other thing I noticed is that trpc will not batch queries unless they belong to the same client.

So instead of this:

export const load: Load = async ({ params, fetch }) => {
  const [authors, books] = await Promise.all([
    trpc(fetch).query('authors:browse', params.id),
    trpc(fetch).query('books:list'),
  ]);
  // ...

You'd do this (if you want to benefit from query batching):

export const load: Load = async ({ params, fetch }) => {
  const trpcClient = trpc(fetch);
  const [authors, books] = await Promise.all([
    trpcClient.query('authors:browse', params.id),
    trpcClient.query('books:list'),
  ]);
  // ...

Regarding your suggestion

I think your second snippet is on the right track. But it doesn't fix the duplication because the first two requests need the same url (/trpc). After the first two requests (1 on server, 1 on client), we should be able to keep using the same client instance (I think).

If we do that, we should keep in mind that sveltekit's fetch is what enables us to use the same relative url. Without it, we'd have to always check const url = browser ? '/trpc' : 'http://localhost:3000/trpc'

thenbe commented 2 years ago

Perhaps @KATT would know if it's currently possible to pass in fetch to an existing trpc client without creating a new instance. Or, if it's not currently possible, what we could try to make it happen.

And if it's not possible at all, whether the overhead for creating a new trpc client for each request is negligible or something to worry about.

icflorescu commented 2 years ago

I think your second snippet is on the right track. But it doesn't fix the duplication because the first two requests need the same url (/trpc). After the first two requests (1 on server, 1 on client), we should be able to keep using the same client instance (I think).

You're right.

thenbe commented 2 years ago

There might me a much simpler way we can get the desired behavior.

// client/trpc.ts
const client = trpc.createTRPCClient<Router>({
    url: '/trpc',
    transformer: trpcTransformer,
});
export default client;
// index.svelte
export const load: Load = async ({ params, fetch }) => {
  trpc.runtime.fetch = fetch; // <-- This
  trpc.query('authors:browse', params.id),
  // ...

Granted, I haven't tested this beyond some basic functionality on my machine, but at first glance querying & batching work seamlessly.

There is another issue though. runtime's type signature is marked as readonly, and I'm not sure why.

Summary:

Benefits of this approach:

  1. No more duplicate trpc clients.

    • Because fetch property of the existing client instance is updated rather than create a new instance on every query.
  2. No more duplicate queries in ssr mode.

    • sveltekit's fetch -> enables use of relative url -> which then enables sveltekit to avoid a duplicate network request in the client

Cons:

  1. Type error because runtime is marked as readonly.
  2. Apporach hasn't been tested sufficiently.

Anyways, it's something we can explore if we want to.

thenbe commented 2 years ago

After some testing, I wouldn't recommed the approach I outlined in my previous comment:

trpc.runtime.fetch = fetch

While this approach may seem to work fine initially, you'll run into unwanted behavior where the trpc client instance is shared amongst different sessions/users. To see this in action, add authz to your trpc router, and run some parallel tests simulating users with different authz rules (using playwright, etc). You'll notice then that your auth rules/middleware/context is suffering from cross contamination between different users/sessions.

I have a feeling it could be related to the part in the docs which states:

Mutating any shared state on the server will affect all clients, not just the current one.

Instead, use the approach outlined in this pr, which works perfectly fine.