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

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

Open
samualtnorman opened this issue Jan 7, 2025 · 4 comments

Comments

@samualtnorman
Copy link

samualtnorman commented Jan 7, 2025

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
Copy link
Author

"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.

@ryansolid
Copy link
Member

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.

@samualtnorman
Copy link
Author

samualtnorman commented Jan 7, 2025

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.

@ryansolid
Copy link
Member

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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants