stytchauth / stytch-node

Official Stytch Backend SDK for Node.js
https://stytch.com
MIT License
90 stars 21 forks source link

Need custom fetch options for native fetch #262

Closed devjmetivier closed 1 year ago

devjmetivier commented 1 year ago

Frameworks like Next.js modify the native fetch implementation to add caching strategies to individual requests (server side). This, in the case of the Stytch Node SDK, creates an issue when making native fetch requests.

For example:

A call using the SDK such as:

const loginOrCreateResponse = await stytch.otps.email.loginOrCreate({ email });

... will work fine on the first attempt, and the user will receive their OTP code. However, if a second request is made with the exact same parameters (email in this case), it will result in a cache hit in Next.js/Vercel and the request won't actually go out to Stytch to complete the request, but instead returns a cached response. No second OTP code is sent.

https://github.com/stytchauth/stytch-node/blob/2479bd3877e66ee745d6ff6607751626112384a7/lib/shared/client.ts#L55-L60

☝️ Here is where the fetch configurations are created. For Next.js, special options need to be passed in order to prevent the above behaviors from occurring.

Please let me know if a reproduction is required, or if it's preferable I can make a contribution to the SDK to add the ability to pass custom fetch arguments.

Note: I might be missing some configuration I can pass to a custom undici agent, but I can't seem to find the right configuration if that's possible.

max-stytch commented 1 year ago

Heya @devjmetivier - thanks for opening the issue. @chris-stytch actually ran into this internally the other day 😞. As an immediate workaround, you can modify the fetchConfig directly by casting the client as type any annotations - e.g.

function monkeyPatchStytchClientSettings(client: ...) {
  /* eslint-disable */
  const cl = <any>client;
  /* eslint-enable */
  cl.fetchConfig.cache = 'no-store';
}

It's a little icky, but it should do the trick.

You can also call a dynamic function like headers() to disable the fetch cache for an entire route, if that is preferred.


Regarding changing the Stytch SDK itself - I have a few questions around implementation that I'll try to get answered:

devjmetivier commented 1 year ago

@max-stytch

Are there other SDKs or libraries that have experienced this same issue?

How do they expose custom fetch options

Auth0's Node SDK experiences the same issue, but they allow a custom fetcher (as opposed to a custom config for fetch).

Details on how they do that 👇 Here they start by adding `fetch` to their default configuration options in types https://github.com/auth0/node-auth0/blob/master/src/lib/models.ts#L8-L42 And this moves up the chain eventually to their Auth Client options... https://github.com/auth0/node-auth0/blob/master/src/auth/base-auth-api.ts#L17-L25 https://github.com/auth0/node-auth0/blob/master/src/auth/index.ts#L12-L22 And finally gets passed down the chain where they'll either use your custom fetcher, or native fetch: https://github.com/auth0/node-auth0/blob/master/src/lib/runtime.ts#L24-L52

Does it make more sense to expose global fetch options on the new Stytch constructor, per-method options, or both?

This is the part where I would inject my opinion. Rather than sending along fetch config options, I'd prefer to pass my own custom fetcher entirely. But I also recognize the following:

  1. A quicker fix is likely more necessary as I assume more will experience this issue, so the ability to pass the fetch config would be preferable (all that's technically needed to fix this is the ability to modify the cache property in the fetch config)

    Note: I'm unsure if anyone would need what Next.js adds to fetch. The next property is the only thing they add. Not sure how that would work.

  2. I'm not sure where this leaves the SDK's use of undici, or what the implications of adding custom fetchers would have for it.

I'm also not even sure what the purpose of undici is in the Stytch SDK anyways The `fetch` implementation from `undici` is never actually used, so I'm not sure what passing a custom dispatcher even does here. I might be missing something 🤷‍♂️ https://github.com/stytchauth/stytch-node/blob/a0eae9b99caca56c5cec36870034cb3aa67bb45e/lib/shared/index.ts#L36 [Undici docs example using their custom fetch implementation](https://github.com/nodejs/undici#undicifetchinput-init-promise)

TLDR; actual answer to your last question:

Fetch options should just go on the new Stytch constructor, unless there's a specific use case for doing both I wouldn't waste the effort.

max-stytch commented 1 year ago

I'm not sure where this leaves the SDK's use of undici

We don't use undici directly. On Node environments, undici is the backing library for globalThis.fetch, and we allow passing custom Dispatchers to it for configuration of telemetry, SSL certificates, HTTP Keepalive, etc.

I'd prefer to pass my own custom fetcher entirely.

That's probably the most robust solution for power users. We were hoping we could avoid the complexity of custom fetchers now, since Node 18 made the fetch API globally available and we were able to drop support for Node 16 earlier than expected. I don't like how custom fetchers put the burden on the caller. Ideally we'd just work in NextJS environments, and we wouldn't require every caller on NextJS 13 to implement the same monkeypatching approach.

unless there's a specific use case for doing both

There are valid API calls that could be cached - for example, stytch.users.get({ user_id }) is a great candidate for caching. But any call that is a POST, PUT, or DELETE is a bad candidate.

In https://github.com/stytchauth/stytch-node/pull/266 we're experimenting with adding custom options to requests. If we stick with that approach, passing in cache options or additional fetch options would be quite nice.

max-stytch commented 1 year ago

Related: https://github.com/supabase/supabase-js/issues/725

devjmetivier commented 1 year ago

@max-stytch

We were hoping we could avoid the complexity of custom fetchers now

Totally. I don't personally/professionally use custom fetchers often anyways. Sticking with adding custom fetch config options for now is a great start.

There are valid API calls that could be cached - for example, stytch.users.get({ user_id }) ...

This is a great point. The only thing I'll add is that cache ought be opt-in for auth related requests. Modifying the default cache behavior of fetch to be no-cache regardless of the environment/framework the SDK is being used in is probably a safe bet. Food for thought 🍜

max-stytch commented 1 year ago

Modifying the default cache behavior of fetch to be no-cache regardless of the environment/framework the SDK is being used in is probably a safe bet.

I agree completely. https://github.com/stytchauth/stytch-node/pull/271 should do just that and resolve the immediate issue with NextJS v13+. I'll open up a separate issue to track if anyone does want to cache responses, and would like the ability to pass arbitrary options into native fetch.