-
Notifications
You must be signed in to change notification settings - Fork 146
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
Update http crate to 1.0 #407
Conversation
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.
rust-version = "1.67"
in Cargo.toml
ought to resolve these failures.
I'm not sure this will fix the error. The CI workflow has the tests set for rust 1.64. |
I would rather inline the one type that's used from |
Ok, changed the PR title and inlined the http method code, removing the http dependency. |
Note: The failing and the requirement to increase MSRV is not due to http (MSRV is 1.49) but due to "toml_edit v0.19.15", which in turn is not related to the change in gloo-net. |
I would propose a new PR to fix this, something like this: diff --git a/crates/worker-macros/Cargo.toml b/crates/worker-macros/Cargo.toml
index 474cd46..d560678 100644
--- a/crates/worker-macros/Cargo.toml
+++ b/crates/worker-macros/Cargo.toml
@@ -19,6 +19,8 @@ proc-macro-crate = "1.2.1"
proc-macro2 = "1.0.47"
quote = "1.0.21"
syn = { version = "2.0.15", features = ["full"] }
+# Required to keep msrv at 1.64
+toml_edit = "=0.19.0"
[dev-dependencies]
trybuild = "1" |
The lockfile is present for that. It's most likely that since you updated it before, the lockfile has the previous entry. You can try reverting the change to Cargo.lock and then rebuilding. That should fix the issue |
Done. |
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.
There are some things in the http crate that may be unnecessary here. Can you ensure we have only what's needed
Reactor to be just the required for gloo-net to work.
Minimum Method version to work with gloo-net. |
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.
Can you also update the CHANGELOG.md accordingly?
This mergeable now? |
crates/net/src/http/method.rs
Outdated
/// HTTP methods that can be used in a fetch request. | ||
/// The methods as defined by the [fetch spec](https://fetch.spec.whatwg.org/#methods). | ||
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] | ||
pub enum Method { |
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 prefer how the http crate handled it. We should use a struct with associated constants and not enum as you can choose your own http verb.
See: #312 (comment)
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 so sure if that's a good idea. Custom HTTP verbs (as far as I know) are not standard. Does fetch even support them?
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 decide what is the better way, I can make the change.
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 is supported and documented on MDN.
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.
If you guys want I can use the http crate, I removed as per request by @hamza1311. I made this pull request, because there is interest in updating leptos in the future to use axum 0.7.x, and there are lots of moving parts, including gloo-net.
@futursolo's comment is on point and reasonable (#407 (comment)). The Perhaps we could move forward using the |
Ok, I will change the PR to use the http 1.x again, it's simple.
Saber Haj Rabiee ***@***.***> escreveu no dia segunda,
18/12/2023 à(s) 16:23:
… @futursolo <https://github.com/futursolo>'s comment is on point and
reasonable (#407 (comment)
<#407 (comment)>). The
http crate is compatible with the current implementation of fetch. By
continuing to use the http crate, gloo-net can likely avoid introducing
breaking changes that would impact users who are already utilizing the
http crate and its custom methods like RequestBuilder::method (
https://github.com/rustwasm/gloo/blob/64d81f1b8f919a016a115484efc7963fab2fa4e9/crates/net/src/http/request.rs#L130C8-L130C8
).
Perhaps we could move forward using the http crate?
—
Reply to this email directly, view it on GitHub
<#407 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAU7OBFZIZDYZX4V4FXTOGDYKBUXHAVCNFSM6AAAAAA74QBS2WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRQHE2TMMZYGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Maybe update the title of the PR now that |
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 one change, then this can be merged. Thanks for your work!
This is a breaking change so it needs a minor version update.
This all set now? |
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.
Apologies for the very late review. This looks good. I'll merge it now
A new release with this (and possibly other, currently open, dependency bump PRs) in it would be greatly appreciated! ^^ |
This bumps http to 1.0 and also the gloo-net minor version.