salvo-rs / salvo-cli

Apache License 2.0
43 stars 8 forks source link

Improving salvo-cli #17

Closed prabirshrestha closed 8 months ago

prabirshrestha commented 9 months ago

Seems like salvo-cli is quite basic. Every time I write a new salvo project I copy paste lot of existing code. loco looks pretty great but is for Axum. It is inspired by Rails so has lot of ideas figured out already.

Would be more than happy to help out as necessary.

fankaiLiu commented 9 months ago

Thank you for your feedback on salvo-cli. I agree, it currently offers quite basic functionalities and not provide continual support for projects as effectively . I appreciate your suggestion to look into loco and its features inspired by Rails. I appreciate your support and look forward to potentially collaborating with you. Have a great day! :)

prabirshrestha commented 9 months ago

Here a few things I suggest could help improve the salvo-cli in random order.

Probably worth having something like https://pocketbase.io or https://staticbackend.com and build upon it or just use their schema.

fankaiLiu commented 9 months ago

Thank you for every suggestion, you have made a lot of valuable comments from an experienced programmer's perspective, I will prioritize them by my perspective and send them out to be done and optimized little by little. I have some questions for you.

By the way, I'm a bit busy at work right now, even working 13 hours a day! I'll keep updating quickly when I can. It's a long way to go. Thanks again for the advice!

prabirshrestha commented 9 months ago

My thoughts on sql/db/orm is that salvo-cli only supports one but allows us to bring our own. Here is how I would do it. I will give an example on how one could implement sign up in a generic way. (Pasting code from my old project for reference)

#[async_trait]
pub trait UserService: Sync + Send {
    async fn signup(&self, new_user: &CreateUserRequest) -> AppResult<CreateUserResponse>;
}
#[async_trait]
impl UserService for MyUserService {
    async fn signup(&self, new_user: &CreateUserRequest) -> AppResult<CreateUserResponse> {
        todo!();
    }
}
sqlx = { version = "0.6.1", default-features = false, features = ["any", "sqlite", "mysql", "postgres"] }
sea-query = "0.27.1"
sea-query-binder = { version = "0.2.1", features = ["sqlx-any", "sqlx-postgres", "sqlx-mysql", "sqlx-sqlite"] }
DB using `AnyPool` ```rust pub struct Db { pool: AnyPool, } impl Db { pub async fn new(config: Arc) -> AppResult { let (pool_options, pool_max_connections) = if config.database.url.starts_with("sqlite:") { let pool_options = config .database .url .parse::()? .create_if_missing(true) .foreign_keys(true) .journal_mode(sqlx::sqlite::SqliteJournalMode::Wal) .into(); let pool_max_connections = 1; (pool_options, pool_max_connections) } else { let pool_options = config.database.url.parse::()?.into(); let pool_max_connections = 10; (pool_options, pool_max_connections) }; let pool = sqlx::any::AnyPoolOptions::default() .max_connections(pool_max_connections) .connect_with(pool_options) .await?; Ok(Self { pool }) } pub fn schema_builder(&self) -> &dyn SchemaBuilder { match self.pool.any_kind() { AnyKind::Postgres => &PostgresQueryBuilder, AnyKind::Sqlite => &SqliteQueryBuilder, AnyKind::MySql => &MysqlQueryBuilder, } } pub fn query_builder(&self) -> &dyn QueryBuilder { match self.pool.any_kind() { AnyKind::Postgres => &PostgresQueryBuilder, AnyKind::Sqlite => &SqliteQueryBuilder, AnyKind::MySql => &MysqlQueryBuilder, } } pub fn pool(&self) -> &AnyPool { &self.pool } pub fn is_conflict_error(&self, error: &sqlx::Error) -> bool { matches!(error, sqlx::Error::Database(dbe) if dbe.code() == Some("2067".into())) } } ```
Migration now has to move to rust, but you write once and it works for sqlite, mysql and Postgres. ```rust use crate::{models::user::UserIden, AppResult}; use super::MigrationManager; use sea_query::{ColumnDef, Table}; use sqlx::migrate::MigrationType; pub fn up(manager: &MigrationManager) -> AppResult { let sql = Table::create() .table(UserIden::Table) .if_not_exists() .col( ColumnDef::new(UserIden::Id) .string() .string_len(255) .not_null() .primary_key(), ) .col( ColumnDef::new(UserIden::Username) .string() .string_len(255) .not_null() .unique_key(), ) .col( ColumnDef::new(UserIden::PasswordHash) .string() .string_len(255) .not_null(), ) .col(ColumnDef::new(UserIden::IsLocked).boolean().not_null()) .col(ColumnDef::new(UserIden::IsAdmin).boolean().not_null()) .col( ColumnDef::new(UserIden::ColorScheme) .string() .string_len(255) .not_null(), ) .build_any(manager.schema_builder()); Ok(sqlx::migrate::Migration::new( 20221018052300, "create user table".into(), MigrationType::ReversibleUp, sql.into(), )) } pub fn down(manager: &MigrationManager) -> AppResult { let sql = Table::drop() .table(UserIden::Table) .if_exists() .build_any(manager.schema_builder()); Ok(sqlx::migrate::Migration::new( 20221018052300, "drop user table".into(), MigrationType::ReversibleDown, sql.into(), )) } ```
Queries are also generic that works in sqlite, mysql and postgres. ```rust #[async_trait] impl UserService for SqlUserService { async fn signup(&self, new_user: &CreateUserRequest) -> AppResult { new_user.validate()?; let id = new_id(); let salt = generate_salt(); let password_hash = compute_password_hash(&new_user.password, &Secret::new(salt))?; let is_admin = if self.config.auth.first_user_admin { self.get_total_user_count().await? == 0 } else { false }; let (sql, values) = Query::insert() .into_table(UserIden::Table) .columns([ UserIden::Id, UserIden::Username, UserIden::PasswordHash, UserIden::IsLocked, UserIden::IsAdmin, UserIden::ColorScheme, ]) .values([ id.clone().into(), new_user.username.to_lowercase().trim().into(), password_hash.expose_secret().to_owned().into(), false.into(), is_admin.into(), "dark".into(), ])? .build_any_sqlx(self.db.query_builder()); sqlx::query_with(&sql, values) .execute(&mut self.db.pool().acquire().await?) .await .map_err(|e| { if self.db.is_conflict_error(&e) { AppError::DuplicateUserName(new_user.username.to_owned()) } else { e.into() } })?; Ok(CreateUserResponse { id }) } } ```
This is what I use for migrator. ```rust use sea_query::{MysqlQueryBuilder, PostgresQueryBuilder, SchemaBuilder, SqliteQueryBuilder}; use futures_core::future::BoxFuture; use sqlx::{ any::AnyKind, error::BoxDynError, migrate::{MigrateError, Migration, MigrationSource, Migrator}, AnyPool, }; use crate::AppResult; pub struct MigrationManager { pool: AnyPool, migrations: Vec, } impl MigrationManager { pub fn new(pool: AnyPool) -> Self { Self { pool, migrations: vec![], } } pub fn pool(&self) -> &AnyPool { &self.pool } pub fn any_kind(&self) -> AnyKind { self.pool().any_kind() } pub fn schema_builder(&self) -> &dyn SchemaBuilder { match self.any_kind() { AnyKind::Postgres => &PostgresQueryBuilder, AnyKind::MySql => &MysqlQueryBuilder, AnyKind::Sqlite => &SqliteQueryBuilder, } } pub fn with_migrator( mut self, migrator: impl Fn(&MigrationManager) -> AppResult, ) -> AppResult { self.migrations.push(migrator(&self)?); Ok(self) } pub fn with_migration(mut self, migration: sqlx::migrate::Migration) -> Self { self.migrations.push(migration); self } pub async fn run_up(&self) -> Result<(), MigrateError> { let m = MigrationList(self.migrations.clone()); let migrator = Migrator::new(m).await?; migrator.run(self.pool()).await?; Ok(()) } pub async fn run_down(&self, target: i64) -> Result<(), MigrateError> { let m = MigrationList(self.migrations.clone()); let migrator = Migrator::new(m).await?; migrator.undo(self.pool(), target).await?; Ok(()) } } #[derive(Debug)] struct MigrationList(Vec); impl MigrationSource<'static> for MigrationList { fn resolve(self) -> BoxFuture<'static, std::result::Result, BoxDynError>> { Box::pin(async move { Ok(self.0) }) } } pub async fn migrate_up(pool: AnyPool) -> AppResult<()> { get_migration_manager(pool)?.run_up().await?; Ok(()) } pub async fn migrate_down(pool: AnyPool, target: i64) -> AppResult<()> { get_migration_manager(pool)?.run_down(target).await?; Ok(()) } mod migration_20221018052300_user; mod migration_20221030105700_storage; fn get_migration_manager(pool: AnyPool) -> AppResult { let manager = MigrationManager::new(pool) .with_migrator(migration_20221018052300_user::up)? .with_migrator(migration_20221018052300_user::down)?; Ok(manager) } ```

As for ORMs I'm not a fan of it. While it allows us to move fast initially and makes it easy to refactor in the long run we always land of fighting ORMs. The good part about the solution is one can use custom sql if needed that might be specific to your database and take advantage of it.

As for surrealdb it is quite new. Due to popularity of sql I would suggest supporting sqlx first. Once the cli has stable list of features, it then would make it easy to port to surrealdb if needed. For now I'm ok with just todo!() implementation so I can easily replace it on my side.

For background jobs I use my own https://github.com/prabirshrestha/mq. Currently only Surrealdb backend is implemented. One could create own backend using sql or Redis or cloud queues if needed.

I don't expect this to be done soon anytime, but wanted to just share thoughts as I think there is lot of potential for the cli to make it easy. Every time I create a new salvo project I'm doing quite a lot of copy paste so I think it not just me but others could also benefit here. Best of luck at your work!

fankaiLiu commented 9 months ago

My thoughts on sql/db/orm is that salvo-cli only supports one but allows us to bring our own. Here is how I would do it. I will give an example on how one could implement sign up in a generic way. (Pasting code from my old project for reference)

  • Introduce traits allowing easy change of implementations.
#[async_trait]
pub trait UserService: Sync + Send {
    async fn signup(&self, new_user: &CreateUserRequest) -> AppResult<CreateUserResponse>;
}
  • If I choose bring your own, replace with todo!(). I can use this with surrealdb or any other implantation.
#[async_trait]
impl UserService for MyUserService {
    async fn signup(&self, new_user: &CreateUserRequest) -> AppResult<CreateUserResponse> {
        todo!();
    }
}
  • As for database I suggest to go with sqlx but use AnyPool and sea-query-binder. This will allow you to write sql once but use it for sqlite, mysql or Postgres.
sqlx = { version = "0.6.1", default-features = false, features = ["any", "sqlite", "mysql", "postgres"] }
sea-query = "0.27.1"
sea-query-binder = { version = "0.2.1", features = ["sqlx-any", "sqlx-postgres", "sqlx-mysql", "sqlx-sqlite"] }

DB using AnyPool Migration now has to move to rust, but you write once and it works for sqlite, mysql and Postgres. Queries are also generic that works in sqlite, mysql and postgres. This is what I use for migrator. As for ORMs I'm not a fan of it. While it allows us to move fast initially and makes it easy to refactor in the long run we always land of fighting ORMs. The good part about the solution is one can use custom sql if needed that might be specific to your database and take advantage of it.

As for surrealdb it is quite new. Due to popularity of sql I would suggest supporting sqlx first. Once the cli has stable list of features, it then would make it easy to port to surrealdb if needed. For now I'm ok with just todo!() implementation so I can easily replace it on my side.

For background jobs I use my own https://github.com/prabirshrestha/mq. Currently only Surrealdb backend is implemented. One could create own backend using sql or Redis or cloud queues if needed.

I don't expect this to be done soon anytime, but wanted to just share thoughts as I think there is lot of potential for the cli to make it easy. Every time I create a new salvo project I'm doing quite a lot of copy paste so I think it not just me but others could also benefit here. Best of luck at your work!

Thank you for your reply, I will keep learning and writing better tools. Have a great day! :)

fankaiLiu commented 8 months ago
prabirshrestha commented 8 months ago

Another one I would consider adding is watch mode.

This will allow us to do something like this and when it is compiling the http endpoint will be available and wait for the compilation to finish and start the server.

cargo install systemfd
systemfd --no-pid -s http::8080 -- cargo watch -x 'run'

You can see the sample from here.

Recently published a surrealdb-migrator that I have been using internally in one of my project. This should hopefully unblock surrealdb support.

prabirshrestha commented 8 months ago

Here are few more:

One option to support errors is preserve the state either in query string or use flash as shown here. This will require full page refresh.

If you really want to include some sort of dynamic UI, probably worth using htmx/unploy/alipne (currently) but without other JS files or scripts. I personally haven't used any of these so can't give any preferences, but would use one that has the least size and has good set of features.

static CSS: &str = grass::include!("../static/_index.scss");
cors_allow_origin:
    - "https://salvo.rs"

(If it makes it easy for you to create an new issue with checkmarks as the main comment feel free to do so and I can start commenting there instead and you can lock this issue.)

fankaiLiu commented 8 months ago

Here are few more:

  • Avoid adding JS library if possible. Most likely folks using will use a full blown SPA libraries such as React/Vue/Angular and so on, so I wouldn't bother. Another reason to do so if some may want apps to work with JS disabled specially for non authenticated pages. JS also may be no go for those who use Typescript and have large number of engineers contributing.

One option to support errors is preserve the state either in query string or use flash as shown here. This will require full page refresh.

If you really want to include some sort of dynamic UI, probably worth using htmx/unploy/alipne (currently) but without other JS files or scripts. I personally haven't used any of these so can't give any preferences, but would use one that has the least size and has good set of features.

  • Group templates into folder. Such as as templates/admin/users/list_page.html instead of templates/user_list_page.html.
  • Implement 500 internal server error page.
  • Add support for CSS minification/compliation. Might be can use grass which includes support for compiling SASS files so we get benefits for variables and imports that can be merged to a single file.
static CSS: &str = grass::include!("../static/_index.scss");
  • Use layout pages instead of duplicating code.
  • Don't default cors to the following. could be security issue.
cors_allow_origin:
    - "https://salvo.rs"

(If it makes it easy for you to create an new issue with checkmarks as the main comment feel free to do so and I can start commenting there instead and you can lock this issue.)

Thanks for the added advice; I'll start a new issue!