Returning `never` from `redirect` function makes incorrect behavior

[seahindeniz]

Describe the bug (it's more like a DX problem)

When I use redirect, its return value makes TS to throw a warning and makes you wrote an incorrect code. Like, I get confused if the redirect function is acting like process.exit(0). I know this is unlikely considering that it doesn't actually break the code, but still, the error is also annoying to have it there

Source https://github.com/solidjs/solid-router/blob/517a28e1de64275955f214b0d4d3b94bbf18108c/src/data/response.ts#L24

Your Example Website or App

The root cause is specific enough, IMO

Steps to Reproduce the Bug or Issue

import { redirect } from '@solidjs/router';

function redirectUser(user: User) {
  if (!user) {
    redirect('/login');

    console.log('this will be logged, and the restOfTheCode will be executed');
  }

  restOfTheCode();
}

Expected behavior

Not to throw error. redirect function should probably return the type of the object it returns. If it is an intended behavior, then at least it should return void or undefined or unknown

Screenshots or Videos

No response

Platform

• OS: Windows 11
• Browser: Chrome
• Version: [e.g. 91.1]: 0.13.3

Additional context

Probably, the reload function may need to be evaluated as well. Also, I would have opened up a PR, but I'm not sure why there's a never usage and what gets affected after changing it.

[ryansolid]

The important part is this can't impact the return type of the function that returns it. It is intended that redirect is always thrown or returned as it doesn't do the redirect but returns the redirect. So in a sense the code that is incorrect here would never work anyway. The fact is never is odd but it is a TypeScript trick. We could I suppose make it always throw as well but we did this to give more control. If void works without impacting types would consider it.

[seahindeniz]

Yes it is more like, createRedirect 🌝 but yeah I see the intention now. Redirecting the actual type of the redirect function, could pollute the return type and caller scope may behave incorrect and bring type-level complications. I just tried void, it gets unionized to the return type but that's alright I guess.

Although, I'm just a starter and don't know all the cases yet to use the redirect function, maybe action and cache or any other wrapper function could strip out redirect type from the return type by using infer

[Brendonovich]

maybe action and cache or any other wrapper function could strip out redirect type from the return type

When combined with redirect returning never, they kind of do this.

redirect, json, etc internally create a Response, which action and cache handle as a special case. The fact that they create a Response is irrelevant to you, since the body is just pulled back out and returned to you transparently when necessary (like returning a json() from a cache function). So they handle the runtime aspect of stripping out the Response, and never takes care of the type aspect. It's the only type that, when in a union with other types, entirely disappears. So if you return a redirect in one if branch and a User in another, the function's return type will just be User, which is great since the Response produced by redirect is transparently handled by cache and action.

I can imagine an alternative that uses branded Response types that get filtered out by cache and action, but realistically the only time you'll be returning Response inside cache and action is with redirect, json etc. Given that you must return or throw the Response for it to do anything, I'd say the never type communicates that the returned value is transparently handled pretty well.

If void works without impacting types would consider it

When a void type is returned from a function, it acts fairly similar to undefined was returned. Attempting to filter out a void return type using T extends void will also filter out undefined, which is definitely not what we want.

[seahindeniz]

If a function returns something, it must be explicitly documented. I understand that I have to return the truthy value from the redirect function but as a developer, I have to know that a call expression like redirect returns a value and must be used somehow and somewhere.

In this case

let res;

if (condition1) {
  res = redirect('page1');
  someOperation();
}
else if (condition2) {
  res = redirect('page2');
  someAnotherOperation();
}
else {
  res = redirect('page3');
}

return res;
}

I know this is not a good, clean code and can be optimized, but that's irrelevant. At least developer must be aware of things and shouldn't get an error that says unreachable code. Junior devs can even get far more confused.

If something like redirect function with the return type which states that it doesn't return anything, and have a name that gives the impression of an action, I'd expect it to do that. Much like process.exit as you know it also returns never and any statement after it gets called or it's return type is meaningless. Otherwise, one can get the same confusion as I have, when I created this issue.

So basically hidden behaviors and mental statements is just bad DX. Even if the return value is being stripped away in background, doesn't change the fact. All procedure must be and can be documented and implemented correctly, IMHO.

[Brendonovich]

imo that specific use case is questionable but there's definitely an argument that it should be valid. Here's a demo of how it could work.

[ryansolid]

This is fixed in the next version of the router (v0.14.0). The types from helpers will reflect reality and it will be the cache and action functions doing the narrowing.