-
Notifications
You must be signed in to change notification settings - Fork 1
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
GraphQL plugin #53
base: main
Are you sure you want to change the base?
GraphQL plugin #53
Conversation
} | ||
|
||
self.services.websocket_client.lock().await.init(); | ||
|
||
let action_to_clone = self.action_tx.as_ref().unwrap().clone(); | ||
|
||
tokio::spawn(async move { | ||
client(Some(action_to_clone)).await; | ||
let _ = client(Some(action_to_clone)).await; |
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 think you can also do client(Some(action_to_clone)).await?;
which would allow the error to surface from the result
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'll check that out!
|
||
use crate::components::component::Component; | ||
use crate::components::handlers::HandlerMetadata; | ||
use crate::components::home::{Home, WebSockerInternalState}; | ||
use crate::components::home::Home; |
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.
This is really minor and you definitely don't have to do it, but a style guideline I follow is to prevent what they call "module-name prefixes", which essentially means repeating the module name within your type names, like components::home::Home
or components::component::Component
or components::handlers::HandlerMetadata
. The Go community recommends against this as well. It's generally more concise and readable to use alternatives like components::Home
, components::Component
, or components::handlers::Metadata
.
To do this, I'll often do public exports of specific types within my lib.rs
or mod.rs
files, like this. With things like pub use hooks::Hook;
and pub use tags::Tag;
, it allows me to use more concise imports like inject::Hook
, and inject::Tag
everywhere.
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.
Also, you can collapse a lot of these imports together into one statement, like this. The end result after adopting these two style guidelines would look something like this:
use crate::{
components::{Component, Home, handlers::Metadata},
services::websocket::{Client, Trace},
tui::{Event, Tui},
wss::client,
};
pub fn handle_general_status(app: &mut Home, s: String) { | ||
app.status_message = Some(s); | ||
pub fn handle_general_status( | ||
app: &mut Home, |
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.
The variable name is app
but the actual struct is Home
. That was confusing. 😅 I wanted to see what abort_handlers
looked like, so I pulled up app.rs
and couldn't find it.
app.abort_handlers.iter().for_each(|handler| { | ||
handler.abort(); | ||
}); |
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 took me a minute to wrap my head around this, so I'll summarize here to make sure I understand. When you're showing a status message, you want to set a 5-second clear action. If the status message is changed during that time, you abort the prior clear and reset the timer to 5 seconds anew so that the new message isn't cleared out too soon?
let thread_handler = tokio::spawn(async move { | ||
sleep(Duration::from_millis(5000)).await; | ||
|
||
sender.send(Action::ClearStatusMessage) | ||
}); |
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'm guessing this is a tokio::spawn
independent task because there's no natural point to await
the results to trigger polling and allow the future to progress?
if graphql.is_none() { | ||
return None; | ||
} | ||
|
||
let gql = graphql.unwrap(); |
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.
One thing that isn't immediately obvious - Option
is a return value that exhibits the same question-mark (?
) behavior that Result
types do. You could potentially replace these lines with just let gql = graphql?;
Then, below you can add a question mark to the end of the let query
, let operation_name
, let operation_type
, and let variables
lines below so that you can entirely eliminate the if query.is_none() || variables.is_none() || operation_type.is_none()
block and the .unwrap()
calls underneath.
No description provided.