Skip to content

Conversation

@ealmloff
Copy link
Contributor

@ealmloff ealmloff commented Feb 26, 2025

This PR adds websocket support for server functions that simultaneously accept and emit a stream of serializable items. This requires some breaking changes around the server function trait to model the request and response type together.

TODO:

  • Websocket protocol implementation
  • Http protocol implementation
  • Axum integration
  • Actix integration
  • Server function macro support
  • Reqwest integration

@gbj gbj mentioned this pull request Mar 7, 2025
@gbj
Copy link
Collaborator

gbj commented Mar 7, 2025

I just read through this all and it looks amazing.

Hope you don't mind, I just pushed two commits after reviewing, as these were pretty minimal changes just knocking a few things off the CI list. The only substantive change was that we also support Axum running as WASM on JS Fetch based platforms like Deno, but mio can't be compiled for that target so the ws feature of Axum doesn't work. I've just made it panic under those circumstances, which I think is fine. Your API design here meant that no Axum WS types had to be exposed anyway, so I think this will work fine.

My only significant feedback: The breakages in the server_fns_axum example suggest to me that migrating to this will require a bunch of changes like this:

// from this
#[server(input = Postcard, output = Postcard)]
// to this
#[server(input = PostPostcard, output = PostPostcard)]

It would be nice if there were a way to do this in the macro automatically, given that the macro already has a default method (I think?)

Thanks for all your work.

@ealmloff
Copy link
Contributor Author

ealmloff commented Mar 7, 2025

I just read through this all and it looks amazing.

Hope you don't mind, I just pushed two commits after reviewing, as these were pretty minimal changes just knocking a few things off the CI list. The only substantive change was that we also support Axum running as WASM on JS Fetch based platforms like Deno, but mio can't be compiled for that target so the ws feature of Axum doesn't work. I've just made it panic under those circumstances, which I think is fine. Your API design here meant that no Axum WS types had to be exposed anyway, so I think this will work fine.

Great! WASM compatibility was one of the areas I'm wasn't not sure about. I think now you should be able to implement the server trait yourself if you want to use websockets with wasm on the server

My only significant feedback: The breakages in the server_fns_axum example suggest to me that migrating to this will require a bunch of changes like this:

// from this
#[server(input = Postcard, output = Postcard)]
// to this
#[server(input = PostPostcard, output = PostPostcard)]

It would be nice if there were a way to do this in the macro automatically, given that the macro already has a default method (I think?)

Thanks for all your work.

I'm hesitant to hardcode too much into the server macro. Looking at that server function call if Postcard only implement Encodes and Decodes, I would assume input works for any struct that implements Encodes<T> + Decodes<T>. For now, I just renamed the encoding variants to *Encoding like PostcardEncoding so this is less breaking.

@ealmloff ealmloff marked this pull request as ready for review March 7, 2025 23:20
@gbj gbj added the breaking label Mar 10, 2025
@gbj
Copy link
Collaborator

gbj commented Mar 11, 2025

Thanks for the changes. I think this is in a good state, I've just been ticking off the last random CI things. Unless you have any other thoughts, I'm happy to merge once it's passing.

@ealmloff
Copy link
Contributor Author

I've just been ticking off the last random CI things. Unless you have any other thoughts, I'm happy to merge once it's passing.

Thanks! Looks good to me. I'm planning on some follow up PRs to make the macro logic more modular in the next few weeks

@gbj gbj merged commit ed915f8 into leptos-rs:leptos_0.8 Mar 12, 2025
137 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants