Proposal: Move sync and async features into seperate modules

[Weasy666]

As came up in the discussion on this rwf2/Rocket#1433, do you think it is feasible to move the sync and async features into seperate modules, like reqwest is doing? So that both can be used simultaneously within one workspace. Otherwise it would not be possible to integrate both, as an option the rocket user can choose from, in rocket_contrib.

The current behavior of the two features would force rocket_contrib to decide which of both to integrate and block the other feature from ever be used with it.

[arn-the-long-beard]

Hey @Weasy666

Thank you for your post !!

If we do move sync and async into two modules, does it mean that we have to rewrite all the unit tests as well ?

[Weasy666]

I'm don't know, but i would guess that it's not necessary. Because you still could rename a imported modul with use arangors::blocking as arangors and use it as drop in replacement. I think that is how reqwest works, IIRC.

[fMeow]

Well, I once tried this approach and find myself always comparing the sync and async version to keep these two implementation the same. Whenever a improvement is made on async version, I have to apply the some modification to sync, and I suddenly find the changes can be improved better. So I am reluctant to move back to this approach. I want to write code once and get both async and sync for free.

I have think of an improvement on maybe_async crate to actually copy the async implementation and convert it to a blocking code. But where to put the copied blocking code in? I haven't come up with a satisfying solution yet. Maybe
copy the whole crate and place it under arangors::sync? Maybe I should give it a try.

There is a really bad workaround, that is to use different versions of arangors with different feature gate to keep one async while convert another version to blocking. Say we can add a 0.3 arangors and set the is_sync feature gate, while the 0.4 version use async. I don't think it a reasonable idea but can be a temporary workaround if you really want both async and sync arnaogrs NOW.

BTW, even if we really keep two version of arangors, one sync and the other async, testing is not a issue since we can use a macro to simply copy the unit test and make it blocking.

[SergioBenitez]

@fMeow I'd like to clarify that the crux of the issue is that your crate is currently misusing Cargo features. Cargo features must be additive; any other use of features is incorrect and can break code. In particular, this means that your crate must be able to build with both features enabled and cannot require any exclusivity between features. Code like the following violates this:

arangors/src/connection/mod.rs

Lines 81 to 85 in cfa2ae9

	#[cfg(any(feature = "reqwest_async", feature = "reqwest_blocking"))]
	pub type Connection = GenericConnection<crate::client::reqwest::ReqwestClient>;
	#[cfg(feature = "surf_async")]
	pub type Connection = GenericConnection<crate::client::surf::SurfClient>;

One way to accomplish this for cases like yours is to namespace the variants. as proposed by @Weasy666. This way, both features can be enabled without breakage.

[fMeow]

The underlying problem is much more complicated than an alias of Connection struct. I am working in progress to replace the current async OR blocking feature gate to async AND blokcing one.

[ManevilleF]

If arangors drops surf support the Cargo.toml dependencies could look like:

default = [ "rocksdb"]
blocking = [ "maybe-async/is_sync", "reqwest/blocking"]
cluster = [ ]
enterprise = [ ]
mmfiles = [ ]
rocksdb = [ ]
arango3_7 = [ ]

Note: reqwest is now not optional

The features would be additive even though both async and sync code won't be available at the same time

[fMeow]

This violation of addtive rules of rust feature gate is a pain in arangors for a long time. But now I think addtive rules is no long a obligation with the new resolver in cargo introduced in Rust 1.51.

But for crates that depends on arangors, I think it a better idea to align with the feature gates in arangors.