tursodatabase / libsql-client-ts

TypeScript/JavaScript client API for libSQL
https://docs.turso.tech/sdk/ts/quickstart
MIT License
286 stars 37 forks source link

Allow custom fetch function for `@libsql/client/http` #38

Closed Coretteket closed 1 year ago

Coretteket commented 1 year ago

Currently, when using the client created by @libsql/client/http, the fetch() is implemented by @libsql/isomorphic-fetch. This works beautifully for most use-cases, however, it is limiting when using Next.js 13.

For their latest app router, they decided to override global fetch() with some { next: { ... } } options. By default, it caches all fetches, unless a revalidate time is set. This means that by default, all calls to client.execute() would be cached by Next.js, without any option for a custom revalidation time.

Therefore, I would propose adding a second optional argument to the client.execute() call, which would allow for a custom fetch() implementation. I think the option should be on the execute call, and not (only) on the client initialization, because this would allow for per-query fetch options. For my use-case, this would allow for a call as follows:

const res = httpClient.execute(stmt, (url, init) => fetch(url, { ...init, { next: { revalidate: 0 } })

Of course, this feature would not necessarily be Next.js-only, as a general option for a custom fetch() implementation could benefit many different use-cases where the standard function is not sufficient or not available.

CodingDoug commented 1 year ago

For reference here is the documentation: https://nextjs.org/docs/app/building-your-application/data-fetching#the-fetch-api

Specifically, about caching, it says:

Requests are not cached if:

  • Dynamic methods (next/headers, export const POST, or similar) are used and the fetch is a POST request (or uses Authorization or cookie headers)
  • fetchCache is configured to skip cache by default
  • revalidate: 0 or cache: 'no-store' is configured on individual fetch

So we have a few options here. I'm making no claims about what's best:

  1. I can't tell for sure, but it sounds like the presence of an Authorization header skips the cache. libsql could simply send a dummy bearer value and hope that it bypasses the cache.
  2. Use POST instead of GET to avoid deduping, perhaps conditionally.
  3. The next.js app developer uses fetchCache to configure caching behavior as needed (not sure if this is flexible enough).
  4. libSQL always uses "cache: no-store" to disable caching.

I like the last option because it's a standard, documented fetch API option and isn't likely to cause overhead or do something unexpected.

(Edit: I'm not sure that selectively disabling cache is going to work well in the face of the consistency guarantees that libsql wants to observe. There is probably a better way for applications to cache query results that doesn't interfere with the way libsql wants to work.)

CodingDoug commented 1 year ago

As a followup thought to this - we might also want to send headers with each request to tell any caching proxies between the client and sqld not to cache the response.

honzasp commented 1 year ago

The client only sends POST requests, which, according to the documentation that Doug cited, should not be cached. Did you observe that the client.execute() returned stale results due to caching with Next.js?

honzasp commented 1 year ago

@Coretteket I'm closing this issue, but feel free to reopen it if you encounter any further problems! :)

Fredkiss3 commented 1 year ago

hello @honzasp @Coretteket i was having this same issue and finally pointed it down to a bug in Next.

Next caches POST request by default if the response of the request has a 200 OK status code, even with an Authorization header that libsql uses.

more info : https://github.com/vercel/next.js/issues/52405

honzasp commented 1 year ago

Hi @Fredkiss3, thank you for pointing this out! Could you please check whether including cache: "no-store" in the fetch() option will correctly bypass the Next.js cache? (As Doug mentioned above) If so, then we can update the client to include this option, so that it works out of the box.

Fredkiss3 commented 1 year ago

@honzasp , yes adding a cache: "no-store" does work, but it would cause other problems when used in an environment that do not support the cache option passed to fetch, precisely cloudfare workers. When using the cache option on fetch using cloudfare workers it gives an error "cache" is not defined on RequestInit. There is a discussion as to why cloudfare workers do not support this option here : https://github.com/cloudflare/workers-types/issues/290#issuecomment-1272272756

Anyway this is a bug on nextjs part as it should not cache POST requests normally and especially if the request contains a Authorization header (which it does today), And the libsql http cient is used mostly with the Authorization header, at least the wrapper lib that i use turso does use an Authorization header.

Fredkiss3 commented 1 year ago

Ah it seems POST request deduping is expected : https://github.com/vercel/next.js/pull/52100 , i suppose because they want to cache graphql requests ? But the case of Authorization header is a bug since it is mentionned in the docs to skip the cache :

Requests are not cached if:

  • Dynamic methods (next/headers, export const POST, or similar) are used and the fetch is a POST request (or uses Authorization or cookie headers)

So maybe the first option about sending a dummy Authorization (in case it is not provided) is the best solution ?

But until it is fixed by next i would just patch the libsql to manually add cache: 'no-store' on libsql.

For people curious, the package to patch is @libsql/hrana-client under lib-esm/http/stream.js or lib-cjs/http/stream.js in the method #createPipelineRequest.

CodingDoug commented 1 year ago

One thing we could do is extend the client API to allow specification of a header/value to send with each query. That would let people change the behavior of the HTTP request on a case-by-case basis for any reason without forcing the SDK into something that's not generally good.

It could be something to pass to the config object at the time of creation or a method on the client to change dynamically.

Fredkiss3 commented 1 year ago

@CodingDoug the first option the author of the issue gave is also a good one : allowing the specification of a custom fetch method, either for each execute statement or on the client config could be interesting.

CodingDoug commented 1 year ago

@Fredkiss3 The main problem is the complexity introduced into the API to support that, which doesn't really support the benefit that the entire community of SDK users would gain from that complexity. It would also be somewhat difficult to explain how to integrate (and why most callers of execute wouldn't need this at all). I think a more generalized approach that doesn't complicate the API so much would be better, but what's "best" in any use case could be different.

Fredkiss3 commented 1 year ago

Hello @CodingDoug for a generalized approach, i think adding a way to customize the fetch used for all the requests on the client config (in createClient) is a good compromise, this is direction many wrapper librairies uses.

you can see planetscale's database-js : https://github.com/planetscale/database-js#custom-fetch-function Or appolo client : https://www.apollographql.com/docs/react/networking/advanced-http-networking/#custom-fetching

honzasp commented 1 year ago

I think that passing a fetch function in the Config object given to createClient() function would be quite straightforward and would solve issues like this.

Doug, what do you think? @CodingDoug

CodingDoug commented 1 year ago

Yeah, I was thinking about the config object as well. That doesn't complicate the API. But we'd need completely exhaustive error checking when invoking foreign functions. There are so many things that could go wrong at runtime that would be very difficult for the developer to diagnose without thorough debugging details. Also we'd have to document fully how the SDK places expectations on the function so that there are no misunderstanding about how it'll be used. @honzasp

Fredkiss3 commented 1 year ago

For people using next.js app router, a workaround for disabling fetch caching globally is to set fetchCache = 'force-no-store' on the root layout.

// app/layout.tsx
import * as React from "react";

export const fetchCache = "force-no-store"; // <-- HERE

export default async function RootLayout({
  children,
}: {
  children: React.ReactNode;
}) {
  return (
    <html>
      <head />
      <body>{children}</body>
    </html>
  );
}

You can still override the option on individual fetches if you are doing other fetches anywhere else.

You can also unstable_cache to cache other queries :

import { unstable_cache } from "next/cache";

const usersTag = 'users'

const getData = unstable_cache(
   async function() {
     return await client.execute(`SELECT * from users`)
   },
   [usersTag], // same as tags
  {
    tags: [usersTag],  // same as tags
    revalidate: 10 // <- each 10 seconds 
})

const fetchOtherAPI = async function() {
  return await fetch(`http://...`, { cache: 'force-cache'  });
}