Skip to content
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

Improve filesystem routing and path management #56

Merged
merged 4 commits into from Jan 11, 2023

Conversation

Angelmmiguel
Copy link
Contributor

@Angelmmiguel Angelmmiguel commented Jan 10, 2023

Refactor the logic to load the app routes from the filesystem. Before, we were using functions instead of related struct implementations. The code was not clear and structs were taking mixed responsibilities. For example, we had a public function to retrieve_best_route that takes Routes as a parameter. I converted this into a Routes's method directly.

This is the list of changes I applied:

  • Create a separate folder for the filesystem routing logic (router)
  • Split Routes and Route structs into separate files
  • Move public functions to struct implementations to avoid passing references around to provide a result
  • Replace glob library with wax. This library provides a better support for glob patterns. For example, glob doesn't support multiple options like *.{js,wasm}
  • Update the main entrypoint to use the new APIs. It didn't require many changes.

It closes #3

@Angelmmiguel Angelmmiguel added the 🚀 enhancement New feature or request label Jan 10, 2023
@Angelmmiguel Angelmmiguel self-assigned this Jan 10, 2023
Copy link
Member

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @Angelmmiguel! Nonblocking: I do wonder if we want to revisit visibility of certain structs/functions.

@Angelmmiguel
Copy link
Contributor Author

LGTM, thanks @Angelmmiguel! Nonblocking: I do wonder if we want to revisit visibility of certain structs/functions.

Great catch @ereslibre! I fixed the visibility of normalize_path_to_url and is_in_public_folder methods. Did you find any other that should be changed?

Thanks!

@ereslibre
Copy link
Member

Did you find any other that should be changed?

I was thinking in this part for this specific patch:

diff --git a/src/main.rs b/src/main.rs
index 7272f9f..403ae82 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -19,7 +19,7 @@ use actix_web::{
 };
 use clap::Parser;
 use data::kv::KV;
-use router::Routes;
+use router::routes::Routes;
 use runner::WasmOutput;
 use std::io::{Error, ErrorKind};
 use std::path::PathBuf;
diff --git a/src/router/mod.rs b/src/router/mod.rs
index 39679f8..9d0bed5 100644
--- a/src/router/mod.rs
+++ b/src/router/mod.rs
@@ -1,9 +1,6 @@
 // Copyright 2022 VMware, Inc.
 // SPDX-License-Identifier: Apache-2.0

-pub mod files;
-pub mod route;
-pub mod routes;
-
-pub use route::Route;
-pub use routes::Routes;
+mod files;
+mod route;
+pub(super) mod routes;

However, we could double check all pub exports to identify which ones are really required to be public (I did run a rg 'pub ')

@Angelmmiguel
Copy link
Contributor Author

Did you find any other that should be changed?

I was thinking in this part for this specific patch:

Good point. The only required entity is Routes. I will make it the only public entity in the router 👍

@Angelmmiguel
Copy link
Contributor Author

I rebased the changes with main to solve git conflicts due to Cargo.lock

@Angelmmiguel Angelmmiguel merged commit 28924de into main Jan 11, 2023
@Angelmmiguel Angelmmiguel deleted the 3-improve-path-management branch May 8, 2023 12:02
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.

Improve path management in wws
3 participants