[Bug?]: Server functions that throw or return redirects when called in an action do not redirect

[apatrida]

Duplicates

• [X] I have searched the existing issues

Latest version

• [X] I have tested the latest version

Current behavior 😯

A server function called from an action (that is not the action itself) that return or throw a redirect to not redirect.

Works:

• Action (client or server) => return/throw redirect

Does not work:

• Action (client or server) => calls a server function => return/throw redirect

The latter works only if you manually propagate the response.

For example in an action:

// validate things
return await myServerFunc() // can redirect

But not when just using the response of the server function in other ways:

// validate things
const data = await myServerFunct() // redirect ignored
// do other things
return 'OK'

This means there is no consistent way to have redirects work from server functions.

Expected behavior 🤔

If a server function throws or returns a redirect then the calling action should respect that and propagate the redirect.

Steps to reproduce 🕹

Steps:

1. create a server function that throws a redirect
2. create an action that calls the server function, ignoring the results (do not return the results of the server function back from the action)
3. see that no redirect happened

Context 🔦

No response

Your environment 🌎

No response

[mmmoli]

I think I'm encountering this bug in a different form.

Following the Protected Routes example and I cannot get the redirect to work even when my definition is:

export const getUser = cache(async () => {
  console.log("getUser");
  throw redirect("/");
}, "user");

[frenzzy]

I was able to reproduce the issue using the following route src/routes/redirect.tsx:

import { type RouteDefinition } from '@solidjs/router'
import { cache, createAsync, redirect } from '@solidjs/router'
import { getRequestEvent } from 'solid-js/web'

const exampleAction = cache(async () => {
  'use server'
  const location = new URL(getRequestEvent()!.request.url)
  return redirect(`${location.origin}/target`)
}, 'exampleAction')

export const route = {
  load() {
    // redirect will start working if uncommented:
    // void exampleAction()
  },
} satisfies RouteDefinition

export default function Component() {
  const data = createAsync(() => exampleAction())
  return <h1>Result: {data()}</h1>
}

With the route above, when you navigate to /redirect, it does not redirect you to /target.

Unless you trigger an action with redirect inside route.load or use a relative redirect which starts with / instead of a full URL with the origin, i.e., redirect('/target') works, but redirect('http://localhost:3000/target') does not.

[Brendonovich]

@mmmoli Redirects are ignored inside load functions and that section of the docs is incorrect accordingly, are you calling getUser inside a route/layout as well? https://github.com/solidjs/solid-docs-next/pull/769

@frenzzy This seems like a separate issue, the redirect should happen on the client whether exampleAction is called in load or not. https://github.com/solidjs/solid-router/issues/438

[Brendonovich]

@apatrida looks like the X-Error header which tells the server function handler to throw is only set for error's, I'll make a PR for it to be included with responses too.

[mmmoli]

@Brendonovich thanks for the follow-up. I was, yes.

Question: will your PR fix error handling for load functions too, or is that by design?

I'll keep an eye on the docs to see what the behaviour is supposed to be.

[Brendonovich]

@mmmoli That redirect should work, is your setup something like this? https://stackblitz.com/edit/github-fk8exj-zbxuey?file=src%2Froutes%2Findex.tsx,src%2Froutes%2Fredirect.tsx,src%2Fapp.tsx

Question: will your PR fix error handling for load functions too, or is that by design?

wym by error handling? i don't think load functions are meant to handle errors at all

[mmmoli]

@mmmoli That redirect should work, is your setup something like this? https://stackblitz.com/edit/github-fk8exj-zbxuey?file=src%2Froutes%2Findex.tsx,src%2Froutes%2Fredirect.tsx,src%2Fapp.tsx

Question: will your PR fix error handling for load functions too, or is that by design?

wym by error handling? i don't think load functions are meant to handle errors at all

Yes? Forked with more specifics: https://stackblitz.com/edit/github-fk8exj-waqwjq?file=src%2Fcomponents%2Fuser.tsx

I'm brand-new to solidjs so might have a different mental model.

Features:

• profile is a protected page
• <User /> attempts to render information on the profile page
• '/nope' to redirect to
• route load function (Does this make any sense at all? Is it even being called?)

[Brendonovich]

@mmmoli The stackblitz you provided seems to be working as expected, and the behaviour isn't affected by the presence of load.

• Clicking the button starts the navigation to /profile
• getUser runs inside createAsync, which suspends the navigation
  • Whether or not getUser is first ran in load or not doesn't affect this
• At the end of getUser the redirect is thrown, which navigates to /nope

If you do think there's a bug there I'd suggest opening a new issue with a dedicated reproduction, I'm not sure it's related to this bug.

[ryansolid]

Ok yeah this was an oversight that I probably introduced towards the end of the beta phase. We definitely should be throwing Responses that were thrown. It is interesting if we should always be throwing them. The types suggest we should but maybe the types are wrong. I see this going one of 2 ways.

1. We keep server functions "transparent". You return a Response, you get a Response. To be fair this will include some special processing of the Response by server functions. We will probably have special Response types from the Router helpers which aren't exactly the standard one.. but like type the body as a generic. We also probably need to do some special handling of the custom body processing. I think with a little work we can make this work although right now we do use the existence of customBody to make decisions, but I think we could find other ways. The important part is that the cache and action functions would be the one stripping the Responses out of the types. Which I think is probably more accurate.

2. We always throw Responses when they are sent across the network. We used to and for some reason I changed it to a return at some point. There is no issue here really other than the caller of the server function that returns a Response maybe not expecting it. Since we don't expose the custom Response stuff we don't really really need to worry about it too much and the developer basically can ignore it. In some ways this is easier for everyone but its less accurate. It is also less extensible.. like if for some reason you wanted to send a Response you couldn't perhaps.

In any case since this takes work my gut is to just merge the current PR for now so that the immediate issue is resolved. And then probably make a choice and do a minor release of the router and/or Start. But would appreciate thoughts/feedback on the direction. I think option 1 is better long term if I can figure out because it makes this into more of a standard protocol rather than a special cased hack. But sometimes hacks work really nicely.

[Brendonovich]

imo option 1 is better, seems more predictable and typesafe. 'You return a Response, you get a Response' sounds more inline with the isomorphic nature of server functions.

We will probably have special Response types from the Router helpers which aren't exactly the standard one.. but like type the body as a generic The important part is that the cache and action functions would be the one stripping the Responses out of the types

This would definitely be doable, I made a small symbol-based implementation the other day but it could just as easily be done with a custom Response class.