-
Notifications
You must be signed in to change notification settings - Fork 8
Added UI framework template #321
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
base: master
Are you sure you want to change the base?
Conversation
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.
Same comments on #320 general review comment holds here
src/main.rs
Outdated
"echo", | ||
"fibonacci", | ||
"file-transfer", | ||
"framework-process" |
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.
Let's change name here and throughout to hyperapp-<something>
. As mentioned on #320, we're going to adopt a convention that hyperapp templates are prefixed by hyperapp-
.
#[http] | ||
async fn submit_entry(&mut self, value: String) -> Result<bool, String> { | ||
self.state.insert(value.clone(), value); | ||
Ok(true) |
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.
As noted on #320, I think these are separated from the local/remote tagged functions in order to demonstrate you can do it. However, in this case, its not actually functional, and so is not demonstrating good practice. Ideally these would do something different than the local/remote functions so that the separation of functions is sane and not just for demonstration purposes
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 am not sure I understand what you mean by 'it's not functional'. FE requests are routed properly (i.e. you can submit an entry you define on the FE). Is this in reference to: https://discord.com/channels/1257731563374776341/1336772246089371678/1364667858801655878?
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 mean submit_entry is the same as add_to_state and view_state is the same as get_state. The separation into a local/remote and separate http function is not functional. As opposed to, say, a case where a local/remote function did thing A whereas http function did thing B
"http-server:distro:sys", | ||
"http-client:distro:sys", | ||
"vfs:distro:sys", | ||
"homepage:homepage:sys" |
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.
Sort, as on #320
@@ -0,0 +1,69 @@ | |||
use hyperprocess_macro::hyperprocess; | |||
use serde::{Deserialize, Serialize}; | |||
use std::collections::HashMap; |
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.
Prefer grouping args like
stdlib
imported in Cargo.toml
and possibly a third group of "written by us"
with each group alphabetized.
So here should either be:
use std::collections::HashMap;
use hyperprocess_macro::hyperprocess;
use serde::{Deserialize, Serialize};
or
use std::collections::HashMap;
use serde::{Deserialize, Serialize};
use hyperprocess_macro::hyperprocess;
thiserror = "1.0" | ||
url = "2.5.3" | ||
rand = "0.8" | ||
wit-bindgen = "0.36.0" |
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.
Get rid of unused deps (I suspect at least bincode, rand, rmp-serde, thiserror, url are unused) & sort remaining deps alphabetically
|
||
// Local Hyperware request | ||
#[local] | ||
async fn add_to_state(&mut self, value: String) -> Result<bool, String> { |
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.
We should fix the caller-utils/wit-generator parsing so we can have a ()
here
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.
Do you mean an function with no argument?
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 mean it should return Result<(), String>
07eb003
to
2bc2260
Compare
Problem
Lack of UI template for App framework.
Solution
Implemented the template.
Docs Update
TODO
Corresponding docs PR
Notes
Tests not working.
{Any other information useful for reviewers.}