Move HandlerType to spin-http
#2863
Conversation
Signed-off-by: Ryan Levick <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
crates/factors-executor/src/lib.rs
Outdated
| @@ -211,6 +211,16 @@ impl<'a, T: RuntimeFactors, U> FactorsInstanceBuilder<'a, T, U> { | |||
| pub fn factor_builder<F: Factor>(&mut self) -> Option<&mut F::InstanceBuilder> { | |||
| self.factor_builders().for_factor::<F>() | |||
| } | |||
|
|
|||
| /// Returns the underlying wasmtime engine for the instance. | |||
| pub fn engine(&self) -> &spin_core::wasmtime::Engine { | |||
There was a problem hiding this comment.
Slightly confusing because in this same crate FactorsExecutorApp::engine() returns a spin_core::Engine. Maybe:
| pub fn engine(&self) -> &spin_core::wasmtime::Engine { | |
| pub fn wasmtime_engine(&self) -> &spin_core::wasmtime::Engine { |
There was a problem hiding this comment.
I agree. The change seems like a fine one to make. While we're here though - a larger put perhaps cleaner change might be to rename spin_core::Engine to spin_core::WasmRuntime. This might be preferrable since even with your suggested change, a user still has to know how to disambiguate when a type signature includes an unqualified Engine.
There was a problem hiding this comment.
or maybe SpinEngine? "Runtime" is also awfully overloaded...
There was a problem hiding this comment.
yet another alternative would be to re-export wasmtime::Engine as spin_core::WasmtimeEngine
Signed-off-by: Ryan Levick <[email protected]>
| percent-encoding = "2" | ||
| routefinder = "0.5.4" | ||
| serde = { workspace = true } | ||
| spin-app = { path = "../app", optional = true } | ||
| spin-locked-app = { path = "../locked-app" } | ||
| tracing = { workspace = true } | ||
| wasmtime = { workspace = true } |
There was a problem hiding this comment.
This should, I think, be under the runtime feature. We have consumers of this crate who use it for things like route parsing (e.g. the cloud plugin), but do not want to drag the whole of wasmtime along with it.
(I'm happy if you prefer to reorganise things to avoid the feature thing, but I really really want a way for cloud-plugin to get what it needs without including wasmtime. Whether that is features or separate crates, I don't care, but it makes a big difference!)
There was a problem hiding this comment.
Ah, I just tried updating cloud-plugin and it looks like some of the other recebt changes have also brought in wasmtime dependency. I'll get a PR out for all of it together.
There was a problem hiding this comment.
Okay, I did some more investigation and I'm gonna punt on it for now. The wasmtime dependency makes 1 min of difference to a clean cloud-plugin build, but not enough difference to an incremental build to be worth the candle, and we are churning cloud-plugin dependencies a lot more slowly than when it drove me to extremes. Something to maybe think about how we can streamline in a more sustainable way in future, but not worth fretting about for now.
Sorry for bending your ear @rylev!
There was a problem hiding this comment.
This is a really good point, and something I would like to start getting a better handle on. Even though it doesn't make a huge difference, I can take a look at fixing this.
This is useful functionality that other runtimes can use to determine which handler export they should call. In the future, we probably want to move calling of the right export completely into
spin-http, but that can be done as a follow up.