Further extend the axum API to support embedded files through RustEmbed. - #4715
metatoaster wants to merge 11 commits into
Conversation
055bcf8 to
ec9e254
Compare
5442007 to
24fc3f4
Compare
Provide a placeholder mode to allow the service of site pkg from bytes directly integrated into the binary.
Ensure `RouterConfiguration` is in fact feature parity with the combined `file_and_error_handler` in the default configuration, where all assets are served from the root (e.g. `favicon.ico` on the root is available). However, it also be possible to change where the assets are served from, or be able to selectively serve specific assets e.g. only `favicon.ico` is made available. Both these configurations may be found in the `start-actix` template.
This avoids forcing the end users from dealing with mode types and just rely on the method names to describe what's being configured. The mode types are now made private. Added a new constructor that will provide the assets by default and that results in a configuration that serves the site root at `"/"`. The existing `new` method will simply provide just the favicon (to be added in a later commit).
- This is to potentially allow the default features to provide routes to embedded resources using the same type of configuration (just with the variants that uses `ServeDir`).
- Renamed `BuiltIn` to `Embed`. - Still need to complete the `Service` implementation to serve the embeded content.
- As that contains the improvement to allow compilation without the environment variable LEPTOS_SITE_ROOT defined.
This is still very rough, but at least we got something that works and can pass the test, however with a caveat that the front-end must first finish building before the backend given how the files must be available for embedding.
24fc3f4 to
9b3dddb
Compare
|
CI might fail or not depending on how |
- Instead of implementing a substandard file service, take advantage of `ServeDir` directly and implement `Backend` and also implement the required traits for wrappers around the types returned by `RustEmbed`. - This required a version bump of `tower-http` to 0.7.
This is a necessary change to avoid a race condition between the two concurrent builds that run concurrently during `cargo leptos build`. Currently there is no way to ensure the frontend pkg artefacts are available before the server is finished building, a major consequence is that the embedding of artefacts in the server build may simply not include the complete frontend when embedding is specified. In the interim, simply have the frontend be built separately before the backend, but `cargo-leptos` also has another issue where it will unconditionally delete `LEPTOS_SITE_ROOT` for all builds, even if the `--server-only` was specified. For this the additional workaround to use `cargo` directly was done. Though this change also brought an improvement to nextest configuration. As the setup scripts feature now supports the specification of environment variables as of 0.9.131 (brought into existence by yours truly), the batch and shell scripts may now be removed and have what needs to be executed simply be included in the `nextest.toml` file.
14bcabc to
42dda1c
Compare
|
I've cleaned up the potential for CI failures by building the frontend before the server, though I had to avoid using In the mean time this embed feature should be considered experimental, as the standard workflow using |
gbj
left a comment
gbj
left a comment
There was a problem hiding this comment.
I think the direction of this is really good! And I definitely want to include it in 0.9. I left some notes on things I noticed while reviewing.
On the cargo-leptos front... I think either of your suggested fixes sounds fine. (Not removing LEPTOS_SITE_ROOT for --server-only seems like a good idea; I'll put together a cargo-leptos PR for that at the very least, and I don't think it needs to block this PR)
| } | ||
| } | ||
|
|
||
| // TODO FIXME rename this to serve_site_root_service |
There was a problem hiding this comment.
No time like the present if you want to rename it :-)
| impl<APP, CX, SH, S, IV1, IV2> traits::RouterConfiguration<S> | ||
| for RouterConfiguration<APP, CX, SH, S> | ||
| #[cfg(feature = "default")] | ||
| impl<APP, CX, SH, S> RouterConfiguration<APP, CX, SH, S> { |
There was a problem hiding this comment.
I think this impl block needs an SR generic, otherwise it defaults to SR = NoEmbedSiteRoot and the builder methods aren't available for the embed mode? I assume it's just an oversight that didn't get flagged by the compiler because of the default generic. Worth checking for any other instances.
There was a problem hiding this comment.
Yeah good catch on that one, also no idea why I didn't make some of the methods not public, going to need to fill out some of these tests to figure out why I did what I did...
Edit: Oh right, I didn't make the underlying mode public because this is configured elsewhere.
| // The favicon is included. | ||
| assert_favicon_ico(&service).await?; | ||
|
|
||
| // However robots.txt is not included to be served (even though it may be embedded). |
There was a problem hiding this comment.
Not sure I understand this part -- why is it desirable that robots.txt 404 even if it's embedded?
There was a problem hiding this comment.
The default setup is limited in what is routed, only routes the site pkg and the favicon, much like the existing behavior of the usual integration provided by the start_actix template. There is another configuration option that should route the whole site root, but that is covered in the non-embedded case but I really should also include that with the embedded configuration to be comprehensive. Again this was an oversight when I can't remember my trains of thoughts when development was spread over too many weeks.
There was a problem hiding this comment.
Revisiting this review while I am not on a phone sitting outside somewhere: this specific case/limitation here is due to how this implementation limits the "scope" (in terms of the amount of files) being presented or routed to the end user, namely the approach is not a generic fallback handler but rather specific routes, and so resource at LEPTOS_SITE_ROOT may be embedded wholesale but only the pkg specific subroutes (plus the favicon) are actually routed. Naturally the other configuration will be needed.
Also, while continuing on this train of thought, I think it might also be beneficial to rather than exposing RustEmbed directly (albeit through a pub reexport), it might be good to introduce two new derive macro to cover both these cases and have the setup to automatically pick up on it. This one I might have to think a bit more about how to best approach it. Basically, rather than:
#[derive(Clone, Copy, Embed)]
#[folder = "$LEPTOS_SITE_ROOT"]
#[prefix = "/"]
struct SiteRoot;Which has the end user having to remember to include all of those very specific set of derive macros and attributes, this probably is much more preferable (actual name tbd):
// Embed just the site pkg.
#[derive(EmbedLeptosSitePkg)]
struct LeptosSitePkg;
// Embed the whole site root.
#[derive(EmbedLeptosSiteRoot)]
struct LeptosSiteRoot;Both will have other different private traits from which the config helper method may be implemented to do the correct thing for the relevant derive so that the correct sets of routes and/or fallback service may be set up on the underlying axum::Router.
Naturally, this can come later, I don't want this additional work to block the release of 0.9.0 needlessly (e.g. if I can't find time again because the infant/toddler catches influenza again). Since the traits have been sealed we should have the flexibility to introduce additional setup method without causing a semver breaking change to include this addition, though I think I might need to better present the API for the generic RustEmbed, perhaps be a bit clear with the documentation as I do want to simply have the config builder call .embed(...) and the correct thing just happens. Though I am not opposed to having .embed_site_root() and .embed_site_pkg(), too, as this also would help make it clear at a glance. Need to think about this more.
| dioxus-devtools = { default-features = false, version = "0.7" } | ||
| wasm_split_helpers = { default-features = false, version = "0.2.2" } | ||
| rust-embed = { default-features = false, version = "8.12" } | ||
| mime_guess = { default-features = false, version = "2.0" } |
There was a problem hiding this comment.
This adds mime_guess but I don't think the code actually uses it -- maybe stale? It appears in the workplace Cargo.toml and in the integrations/axum Cargo.toml but not in code except in the hackernews_islands_axum example
There was a problem hiding this comment.
Yeah that should be removed, it was part of the naive implementation that I had previously done before realizing that tower_service introduced the Backend trait for ServeDir a bit over a month ago.
|
leptos-rs/cargo-leptos#682 for my humble attempt at a cargo-leptos fix |
This branch has not been deployed
2 participants
In this change I also added a couple other changes that was omitted due to how the setup didn't include a way to also include
favicon.icowithout the rest of the assets, as this is a feature with Lepto'sstart-actixtemplate.Currently a draft as I haven't had time to finish up the service side of things and the tests. I really want to get this in before 0.9 is actually out. At the very least get the favicon.ico in to make it more parity with what people expected with the previous fallback.