Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Generic Server Fn #3397
base: main
Are you sure you want to change the base?
Generic Server Fn #3397
Changes from all commits
3864517
ccc51f1
def448b
9480b8b
62a2f2f
f2d7668
00a8139
3333846
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me that the "default generic" approach is working here — for example, if I try to call it as a function
The compiler errors
Looking at the macro expansion, it looks like the default is not added on the function
but of course, that's because you can't give default values to generics on functions
gives the error
From my perspective, this just makes it confusing: Server functions are intended as an abstraction over functions, but Rust functions don't allow default generics; so now an invalid syntax for a function (including a default generic) is accepted by the macro but stripped from the expansion.
I'll put it this way: I know Rust fairly well, and I did not know any of the above until I tested it. I think this is likely to lead to more user confusion than benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I kind of tacked default on at the last minute as a "wouldn't this be cool" kind of feature. I came up against no default generic functions, I think I must have added it for server function structures but forget to try it against functions (I might have not gone ahead with it if I had lol). I'm not sure this feature makes sense for the reasons you stated i.e given that generics can't be default in server functions, and since the server function struct is where they'd have to be default in but that's not a given for any particular server function it is kind of confusing etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably just remove the default generic possibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise the addition of the
_marker
field here is confusing, I think — this name doesn't come from anywhere, so if I'm the user I don't know that it exists or how to tweak the name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be called _phantom instead?
We need PhantomData for specifying the generic types of the server functions when they otherwise aren't present. I could default to _phantom for field names and add an arg the ServerFnArg that let you specify an alternate _phantom field. My thinking here is that people could write
Maybe we should try to implement default for server fn structures with generic arguments that would otherwise require Phantomdata. But then again, you lose some of the strengths of your type system when you can then start to accidentally pass default arguments to a server function that would really prefer the real thing. Might be hard to debug...
But otherwise I think the existence of PhantomData would have to be communicated to the user via the documentation. Since I don't think there's an ergonomic way around using PhantomData here, alternatively we could propagate the generic canoncalization process to the naming convention (lol sorry) for the structure so for instance where we have
and actually creates two structures
ServerFnName_String ServerFnName_u8, both without arguments.
But that's definitely worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the current implementation end up only needing one
_marker
, or does it need one_marker
per generic type?If it needs one per generic, they could be named after the generic types (
_ty_S
or whatever).If it's just one, I guess
_marker
is fine. I agree with you that it will end up being auto-doc'ed, which is good, and that rust-analyzer and the compiler will help, so I think it's fine ultimately.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it might be better to either 1) allow the user to specify the name of the phantom struct in
ssr_type_shim!
or 2) rename this from___Phantom
to something more like___FromClient
.I would prefer the first option for discoverability/"go to definition" reasons, personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I can do that, would you prefer a mixture of both or to force the user to name it.
The mixture would consume an argument and override the default of ___FromClient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking back at this a month later -- I think it's fine to go with a default of
__FromClient
, and maybe allow the user to override it.