Skip to content
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

Merged
merged 22 commits into from Jan 11, 2024
Merged

Conversation

dgsantana
Copy link
Contributor

This bumps http to 1.0 and also the gloo-net minor version.

Copy link

@tysen tysen left a 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.

@dgsantana
Copy link
Contributor Author

I'm not sure this will fix the error. The CI workflow has the tests set for rust 1.64.

@hamza1311
Copy link
Collaborator

I would rather inline the one type that's used from http crate than bump the version and MSRV while keeping the requirement of pulling in another dependency. I would be happy to look at any PRs removing the dependency altogether.

@dgsantana dgsantana changed the title chore(deps): bump http to 1.0 on gloo-net Inline http crate Method code, removing http dependency Nov 30, 2023
@dgsantana
Copy link
Contributor Author

Ok, changed the PR title and inlined the http method code, removing the http dependency.

@dgsantana
Copy link
Contributor Author

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.
This is due to proc-macro-crate used in gloo-worker-macros.

@dgsantana
Copy link
Contributor Author

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"

@hamza1311
Copy link
Collaborator

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

@dgsantana
Copy link
Contributor Author

Done.

Copy link
Collaborator

@hamza1311 hamza1311 left a 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

crates/net/src/http/method.rs Outdated Show resolved Hide resolved
crates/net/src/http/method.rs Outdated Show resolved Hide resolved
crates/net/src/http/method.rs Outdated Show resolved Hide resolved
Reactor to be just the required for gloo-net to work.
@dgsantana
Copy link
Contributor Author

Minimum Method version to work with gloo-net.

Copy link
Collaborator

@hamza1311 hamza1311 left a 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?

crates/net/src/http/method.rs Outdated Show resolved Hide resolved
crates/net/src/http/mod.rs Outdated Show resolved Hide resolved
crates/net/Cargo.toml Outdated Show resolved Hide resolved
@tysen
Copy link

tysen commented Dec 13, 2023

This mergeable now?

/// 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 {
Copy link
Collaborator

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)

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

@futursolo futursolo Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

This is supported and documented on MDN.

Copy link
Contributor Author

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.

@sabify
Copy link

sabify commented Dec 18, 2023

@futursolo's comment is on point and reasonable (#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?

@dgsantana
Copy link
Contributor Author

dgsantana commented Dec 18, 2023 via email

@tysen
Copy link

tysen commented Dec 23, 2023

Maybe update the title of the PR now that http has returned?

@dgsantana dgsantana changed the title Inline http crate Method code, removing http dependency Update http crate to 1.0 Dec 23, 2023
Copy link
Collaborator

@hamza1311 hamza1311 left a 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!

CHANGELOG.md Outdated Show resolved Hide resolved
This is a breaking change so it needs a minor version update.
@tysen
Copy link

tysen commented Jan 2, 2024

This all set now?

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@hamza1311 hamza1311 left a 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

@hamza1311 hamza1311 merged commit a823fab into rustwasm:master Jan 11, 2024
20 checks passed
@torokati44
Copy link

A new release with this (and possibly other, currently open, dependency bump PRs) in it would be greatly appreciated! ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants