-
Notifications
You must be signed in to change notification settings - Fork 425
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
Axum integration #1088
Axum integration #1088
Conversation
In case this PR will get merged, it would be nice to update the projects documentation and book to point out an integration for Axum exists. |
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.
Some notes
juniper_axum/Cargo.toml
Outdated
|
||
[dependencies] | ||
axum = { version = "0.5.11", features = ["ws"]} | ||
juniper = { path = "../juniper" } |
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 a version required?
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.
@btielen in the same way as other integrations have. The version bump is automated with cargo-release
. See release.toml
files for examples.
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.
Wow, this is an AMAZING PR 🥳 . Thank you for the change, adding docs, and adding tests! I especially like that you improved the ws
stuff while you where here.
Some small nits but it looks solid otherwise.
juniper_axum/CHANGELOG.md
Outdated
|
||
## master | ||
|
||
### BC Breaks |
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.
Just remove this section and put something like:
- Initial release
juniper_axum/CHANGELOG.md
Outdated
|
||
## Previous releases | ||
|
||
See [old CHANGELOG](/../../blob/juniper_warp-v0.0.0/juniper_axum/CHANGELOG.md). |
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 mentions warp
and should be deleted.
juniper_axum/Cargo.toml
Outdated
|
||
[dependencies] | ||
axum = { version = "0.5.11", features = ["ws"]} | ||
juniper = { path = "../juniper" } |
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.
As mentioned in the PR comments already, this should have a version and wired up to the release machinery.
juniper_axum/Cargo.toml
Outdated
juniper = { path = "../juniper" } | ||
serde = "1.0" | ||
serde_json = "1.0" | ||
juniper_graphql_ws = { path="../juniper_graphql_ws" } |
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.
Ditto.
juniper_axum/README.md
Outdated
[Juniper Book]: https://graphql-rust.github.io | ||
[Rust]: https://www.rust-lang.org | ||
|
||
[1]: https://github.com/graphql-rust/juniper/blob/master/juniper_warp/examples/warp_server.rs |
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.
Fix URL.
juniper_axum/examples/simple.rs
Outdated
|
||
let context = Context; | ||
|
||
let app = Router::new() | ||
.route("/", get(|| playground("/graphql", Some("/subscriptions")))) |
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.
@btielen Great job, thanks! I've made some insignificant changes though. Most notably simplifying the usage of the playground
, now it feels to me a lot more axum
y.
juniper_axum/examples/simple.rs
Outdated
@@ -72,20 +74,20 @@ async fn main() { | |||
} | |||
|
|||
pub async fn juniper_subscriptions( | |||
Extension(schema): Extension<Arc<AppSchema>>, |
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.
Also I've removed a need to wrap all Scheme
s into Arc
s.
Thanks for the feedback, I will take a look at it tomorrow. |
👋 Incredible work 🤩 , Is there any plan to merge & release it ? |
Any blockers on merging this? |
Hey, are you going to push Axum integration? I loved Axum when I tried it, and I want to use something I already know when using GraphQL |
There are still outstanding review comments here... |
@btielen are you able to fix those changes so to merge this? |
I plan to finish it in next few weeks. |
# Conflicts: # juniper/src/http/mod.rs # juniper_actix/src/lib.rs # juniper_graphql_ws/src/lib.rs # juniper_warp/src/lib.rs
…ql-ws` protocols
Progress
|
@LegNeato I'm going to merge this in next few days. Take a final look, if you would like. All the stuff is ready, only minor CI issues are left to be resolved. |
This PR provides an integration for Axum including subscriptions (fixes #986)
juniper::http::tests::run_http_test_suite
juniper::http::tests::run_ws_test_suite
Get started
Run
cargo run -p juniper_axum --example simple
from the root directory, and navigate to127.0.0.1:3000
in your browser.Other changes
Minor changes were made in other libraries
Sink
forjuniper_graphql_ws::Connection
, so the code doesn't panic.juniper::http::tests::run_ws_test_suite
to test POST requests with variablesDiscussion
juniper::http::GraphQLRequest
now has a property variables with typeOption<InputValue<S>>
, I think (but not sure) things would be easier if this has the typeVariables<S>
operation_name
definedAny help polishing this PR is appreciated :)