solidjs
[Bug?]: Redirects from nested `cache` server functions get ignored
Duplicates
- [X] I have searched the existing issues
Latest version
- [X] I have tested the latest version
Current behavior 😯
When a cache server function calls another cache server function, any redirects thrown inside the inner function are ignored an undefined is returned.
Expected behavior 🤔
When a cache server function calls another cache server function, any redirects thrown inside the inner function are rethrown by the outer function.
Steps to reproduce 🕹
Reproduction: https://github.com/devagrawal09/solid-start-repro
Context 🔦
I'm trying to define a shared function for auth that can be called on both client and server as needed. This function checks for appropriate headers and throws a redirect if the user is signed out or doesn't have the correct role.
Your environment 🌎
No response
I'm not sure this actually makes sense from a design perspective. Because if we say that we just rethrow then we need to rethrow when it's top-level too which doesn't make sense. There is no one to catch it. cache is supposed to handle the redirects and responses. There no way what it returns could include those unless it knew it wasn't topmost which isn't possible without AsyncContext. Depending on that behavior would be odd without being able to support it as this is a router feature.
Again maybe cache is just the wrong name for this. Nesting calling server functions we can do but recursively calling these doesn't really work.
If anyone has suggestions I'm open to them but I don't really see how this can work.
we need to rethrow when it's top-level too which doesn't make sense. There is no one to catch it
Can you elaborate on this? Do you mean that when a cache server function is called form a regular server function that's not wrapped in a cache? Because in that scenario I would expect the top level server function to simply throw the redirect object to the user
There no way what it returns could include those unless it knew it wasn't topmost
So you are saying that the inner cache function can't know that it's being called inside another cache function, so it can't rethrow or return the redirect correctly?
Again maybe cache is just the wrong name for this. Nesting calling server functions we can do but recursively calling these doesn't really work.
Fair, maybe part of the issue is my expectation to just wrap anything that fetches data with cache and then treating it like a regular server function everywhere, rather than something that's only supposed to be consumed by the router.
I was hoping to make use of cache's deduping functionality on the server, for example, if a server function calls the same getUser function multiple times, I don't want that request made multiple times, and I knew cache was designed to also work on the server to dedupe requests. Is that the wrong expectation? Are the server deduping capabilities only designed to work with server components?
If the cache is only supposed to be used by the router so that redirects are properly handled, it might be useful to not have the return value be an async function that can be called, but rather a CachedFn object that's consumed by a router aware hook like useCache.
Something needs to handle the redirect. Right now the cache catches it and processes the response. If it were to rethrow the caller would receive it too. Ie createAsync etc would receive the thrown response instead of the value instead of the processed value. For a redirect that could be fine because it is kinda over, but generally not.
When I put the rethrow logic in I saw the error on the server. The idea here is that cache catches and handles the response. Something needs to know how to do so. This isn't server function only capability, in fact it knows how to serialize responses but not to do anything with them. It's the client cache wrapper that does when called from the client. And similarly the server cache wrapper on the server.
Adding an additional router wrapped primitive over createAsync also feels undesirable. Handling stuff like preloads/single fight capture means cache still needs to come from the router so useCache doesn't get us anything.
What I think you want is a way to dedupe server only that is independent of the router mechanism.
We really need to rename cache. Something to signify it is router data. Having other server function wrapper seems easy enough. Doesn't even require a key. Too bad it is probably too aggressive to dedupe all server functions with same args over a request.
Maybe .raw property on cache and actions that return the underlying function?
Maybe
.rawproperty oncacheandactionsthat return the underlying function?
I'm not sure that is enough. If that were the case you could just pull the functions out from the wrappers and reference the internal function. I think you want the deduping behavior, right?
What I think you want is a way to dedupe server only that is independent of the router mechanism
Would it not be nice for cache to also fill that role? As the universal data fetching mechanism?
There no way what it returns could include those unless it knew it wasn't topmost which isn't possible without AsyncContext
So what's the issue with this? We could use asynccontext to detect if a cache function is top level or running inside another cache function, and rethrow redirects. Nested cache functions already rethrow as expected on the client, this is only a problem when its a server function, and we already use asynccontext on the server.
I'm not sure that is enough. If that were the case you could just pull the functions out from the wrappers and reference the internal function. I think you want the deduping behavior, right?
Can the catching of errors / redirects and the memoization/deduping not be decoupled? so .raw can still use the memoziation behaviour without the error catching from the cache wrapper. .raw wouldn't be a great name for that but I think something like that would be enough?
Nested cache functions already rethrow as expected on the client.
If this works I'm surprised. Looking at this if we find a Response we handle it in both cases. https://github.com/solidjs/solid-router/blob/main/src/data/query.ts#L189. This return is what short circuits it in both cases.
I'm not sure if the renaming from cache to query helps a bit here in the latest router but I don't think directionally this makes sense anyway. If we want cache functions on the server there probably should be a dedicated method that works consistently and has nothing do with the router.
Yeah I realized that my expectations were misaligned with what cache is supposed to do on the server. Thanks for the explanation. I'm good with closing this.