Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need custom fetch options for native fetch #262

Closed
devjmetivier opened this issue Oct 13, 2023 · 6 comments · Fixed by #271
Closed

Need custom fetch options for native fetch #262

devjmetivier opened this issue Oct 13, 2023 · 6 comments · Fixed by #271
Assignees
Labels
bug Something isn't working

Comments

@devjmetivier
Copy link

devjmetivier commented Oct 13, 2023

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.

this.fetchConfig = {
baseURL: config.env,
headers,
timeout: config.timeout || DEFAULT_TIMEOUT,
dispatcher: config.dispatcher,
};

☝️ 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
Copy link
Contributor

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:

  • Are there other SDKs or libraries that have experienced this same issue?
  • How do they expose custom fetch options?
  • Does it make more sense to expose global fetch options on the new Stytch constructor, per-method options, or both?

@max-stytch max-stytch self-assigned this Oct 16, 2023
@max-stytch max-stytch added the bug Something isn't working label Oct 16, 2023
@devjmetivier
Copy link
Author

devjmetivier commented Oct 16, 2023

@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.

  1. 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 🤷‍♂️

response = await fetch(url.toString(), {

Undici docs example using their custom fetch implementation

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
Copy link
Contributor

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 #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
Copy link
Contributor

Related: supabase/supabase-js#725

@devjmetivier
Copy link
Author

@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
Copy link
Contributor

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. #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants