solid-router icon indicating copy to clipboard operation
solid-router copied to clipboard

The current design of `"use server"` and combined with `query()` is inherintly type unsafe

Open samualtnorman opened this issue 1 year ago • 5 comments

Describe the bug

The expected usage of query() with SolidStart's "use server" looks something like this:

const getUser = query(async (email: string) => {
	"use server"
	// `email` is typed as a `string` here which makes sense because that's what it's declared as in the parameter
	// ...
}, `get-user`)

In the client code the DX is very good, when you go to use getUser() you will get type errors for passing in the wrong type and the return type of getUser() can be automatically inferred.

The problem with this however is that once you cross the boundary from the client to the server, the passed in data can no longer be trusted. email might not be a string yet that's what it's typed as, this is type unsafe and could potentially lead to people writing security vulnerabilities.

Something I do in the codebase for the company I work at's app is to do something like this:

const getUser = query(async (email: unknown) => {
	"use server"
	// `email` is typed as `unknown` which is good because it will force me to validate it first before using it
	// ...
}, `get-user`)

This comes with its own problems though, the return type is still inferred which is good but the client is allowed to accidentally pass in a number and typescript won't let you know you made a mistake because number is assignable to unknown.

This problem currently does not have any easy workarounds that I'm aware of.

Ideally there should be some middle ground, like maybe query() could let you pass in a type parameter that decides what the argument types are on the outside, but inside the function the parameters are still typed as unknown and the inferred return type stays intact.

Your Example Website or App

see code examples above

Steps to Reproduce the Bug or Issue

see code examples above

Expected behavior

For the default way of writing "use server" query()s to be type safe.

Screenshots or Videos

No response

Platform

@solidjs/meta 0.29.4 @solidjs/router 0.15.2 @solidjs/start 1.0.10 solid-js 1.9.3

Additional context

No response

samualtnorman avatar Jan 07 '25 15:01 samualtnorman

"use server" on its own is also type unsafe but that seemed like a harder design issue to be solved in the SolidStart repo. I imagine query() combined with "use server" can solved in the router.

samualtnorman avatar Jan 07 '25 15:01 samualtnorman

Hmm.. I think from this perspective "use server" is completely the problem. query can be typed safely. It is only "use server" where guarantees can't be met. Although it is sort of by design so this is a tricky area. I don't really see how to fix server functions without wrapper functions but I don't know that the onus can be on every wrapper. It's just the wrong design if you want these sort of guarentees.

I wonder if someone has opened the same issue under React repos. I was aware of variations of this when I made the decions. But I knew that regardless of what API I chose for Solid everyone would be supporting "use server" because of React. When people like Jarred from Bun were asking me about it, it was pretty obvious that this was probably something that was going to be out of our control. So it's kinda by design.

ryansolid avatar Jan 07 '25 16:01 ryansolid

That is a fair assessment. Will you move this issue to the SolidStart repo?

If you are set on satisfying React-brain people I would suggest to have 2 ways of declaring server functions then, one that is closer to the old style and one that is closer to the React style.

I do not know if it is a good idea to continue to copy React's design however as they do not have the same constraints Solid does. I don't think TypeScript is a focus for React (they're not even written in TypeScript) however it does seem to be for Solid.

samualtnorman avatar Jan 07 '25 16:01 samualtnorman

Yeah I expect as we work closer with Tanner and Tanstack this will happen. Supporting "use server" is important so we can support random bundler X or new library Y. It is a lower level primitive that in being constrained by its design stops it from having too many opinions. There are elements of that I really like. But TS is an issue. It's why Tanner did not move to "use server".

ryansolid avatar Jan 07 '25 16:01 ryansolid

How do we feel about using Standard Schema to allow users to inject whatever validator they want? Tanstack Router uses it. I demo a (very crude) impl here. I can flesh it out some more if we like the concept.


I also tried to write a simple wrapper around Zod and solid-router's query:

import { z } from 'zod'

export function safeQuery<TSchema extends z.ZodTypeAny, TOutput>(
	cacheKey: string,
	schema: TSchema,
	fn: (x: z.infer<TSchema>) => Promise<TOutput>,
) {
	'use server'
	type X = z.infer<TSchema>
	return query(async (x: X) => {
		const parsed = schema.parse(x) as X
		return await fn(parsed)
	}, cacheKey) as (x: X) => Promise<TOutput>
}

const myCachedFn = safeQuery('myQueryKey', z.string(), async (x) => {
	return await someServerFn(x); // `x` validated to be a string
})

Unfortunately, depending on where you put use server, this results in ReferenceError: schema is not defined or H3Error: Cannot call server function outside of a request (or Server Functions cannot be nested in other blocks or functions on solid-start v1.1.3).

AlexErrant avatar Mar 03 '25 22:03 AlexErrant