-
Notifications
You must be signed in to change notification settings - Fork 16
Add support for WASI P3 #87
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
base: main
Are you sure you want to change the base?
Conversation
cc/ @alexcrichton @tschneidereit for comments on the general shape of the upstreamable bits and possible next steps on where this could live. |
Signed-off-by: Brian Hardock <[email protected]>
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 don't feel able to comment on the implementation, although what I saw looked very cool - the same kind of "wow this is going to be super convenient" as some of Ryan's early work on the Spin HTTP SDK.
I had some surface level nits and suggestions but in terms of the overall stuff it looks great to me but definitely needs other eyes on it.
Do we want to more-explicitly frame this initial implementation as experimental/unstable (feature flag, |
This commit eliminates the |
f064bcc
to
3dc8d5b
Compare
4e09979
to
0c104dc
Compare
Once bytecodealliance/wasi-rs#126 lands I'll remove the |
Signed-off-by: Brian Hardock <[email protected]>
|
||
impl<T: http_body::Body> IntoResponse for (http::StatusCode, T) { | ||
fn into_response(self) -> HttpResult<types::Response> { | ||
unreachable!() |
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.
Is this intentional? Using unreachable!
rather than todo!()
makes me assume so but maybe add a comment on why it is unreachable?
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.
Oh this one slipped by! Ill fix, thanks for calling it out!
/// The macro expands into a module containing a `Spin` struct that implements the | ||
/// WASI `http.handler/Guest` interface, wiring the annotated function as the | ||
/// handler’s entrypoint. This allows the function to be invoked automatically | ||
/// by the Spin runtime when HTTP requests are received. |
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.
Nit: we should have an # Example
section showing this macro in use.
Error::Other(e) => write!(f, "{e}"), | ||
Error::Response(e) => match http::StatusCode::from_u16(e.get_status_code()) { | ||
Ok(status) => write!(f, "{status}"), | ||
Err(e) => write!(f, "{e}"), |
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 not sure "{e}"
will show a good error message. Perhaps we hardcode it to "invalid status code"
?
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.
Perhaps even "invalid status code {}", e.get_status_code()
? (With e
not shadowed in the match arm, obvs)
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 code is already in the process of being upstreamed: bytecodealliance/wasi-rs#126
Moves the contents of the prototype repo into the sdk.
The additions are factored into 3 new crates:
The contents of these repo are accessible via the
spin_sdk
through thehttp_wasip3
module.Draft for now while I make a final pass to eliminate low hanging TODOs. Nevertheless requesting reviews now.