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

Hotfix: Temporarily disable static type checking for route handler return type #55261

Conversation

joulev
Copy link
Contributor

@joulev joulev commented Sep 12, 2023

Why?

#51394 added static type checking for route handlers return type, so all route handlers must return Response | Promise<Response>.

But the checker cannot detect when the function always throws, for example

export function GET() {
  throw new Error(); // can be notFound(), redirect()
}

because GET is inferred as () => void, not as () => never, adding | never to the type checker can't fix it.

I can't find a way to get this to work (see the "Notes" section below for more details), so this PR disables that entire type checking so route handlers like the above can pass. Hopefully this change is temporary.

How?

Remove the part where the TS plugin checks for the return type of route handlers.

Notes

Without this PR, the following still works, because the return type is made known to TypeScript:

// Explicit return type
export function GET(): Response {
  throw new Error();
}

Allowing never in the TS plugin

-      __return_type__: Response | Promise<Response>
+      __return_type__: Response | Promise<Response> | never

will also make the following work:

// Arrow function
export const GET = () => {
  throw new Error();
}

because GET is inferred to return never. However, using regular function declaration won't work because it is inferred to return void (see microsoft/TypeScript#8767 and microsoft/TypeScript#16608). I can't figure out how to get type checking to work without requiring users to either use arrow functions or explicitly define the return type.

Interested people can go to this minimal TypeScript example and try to fix.

Signed-off-by: Vu Van Dung <me@joulev.dev>
@ijjk
Copy link
Member

ijjk commented Sep 12, 2023

Allow CI Workflow Run

  • approve CI run for commit: 136758e

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

Copy link

@orca-security-us orca-security-us bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Secrets high 0   medium 1   low 0   info 0 View in Orca

@joulev
Copy link
Contributor Author

joulev commented Sep 12, 2023

Assuming TypeScript doesn't fix the inference bug (microsoft/TypeScript#16608 (comment)), which is very likely, I think we either have to disable static build-time type-checking (this PR) or force users to use return types in this case (requiring code updates for some users). Both are bad but I can't see a third way.

@joulev
Copy link
Contributor Author

joulev commented Sep 24, 2023

Closing in favour of #55849.

@joulev joulev closed this Sep 24, 2023
@github-actions github-actions bot added the locked label Oct 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants