-
Notifications
You must be signed in to change notification settings - Fork 138
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
Expose API for providing a custom Connect impl and pass through the runtime
feature
#316
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.
LGTM. I'm just curious if we really need to feature gate so much of the crate. I would have thought that the feature gated functions would not cause any issues for the WASM platform as they are generic over T.
How can we add tests for this? It would be really nice to make sure future additions to this crate don't break WASM support.
@@ -14,7 +14,8 @@ readme = "README.md" | |||
all-features = true | |||
|
|||
[features] | |||
default = ["rt_tokio_1"] | |||
default = ["runtime", "rt_tokio_1"] | |||
runtime = ["tokio-postgres/runtime"] |
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.
How about making this a dependency of the rt_tokio_1
and rt_async-std_1
feature instead? That way the default feature set will just stay the same and it won't break for users of rs_async-std_1
either.
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 like this idea! Will update the PR.
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.
Or actually, on second thought, this won't work unfortunately. Because for example on WASM, we do want rt_tokio_1
if we're running on Tokio, just not the runtime
feature.
@@ -82,6 +89,7 @@ pub struct Manager { | |||
} | |||
|
|||
impl Manager { | |||
#[cfg(feature = "runtime")] |
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.
Do you really need to feature gate all of this? As the feature gated functions are generic over T
it shouldn't cause any harm, does it? Is the core of the issue the tokio::spawn
which isn't supported by the WASM target?
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.
So this one (and its friends) need to be gated because they all initialize the Postgres connection through the tokio_postgres::Config
. That's what's not supported when you don't have the runtime
feature on tokio_postgres
(like on WASM), since it requires tokio_postgres
to establish the TCP connection on your behalf. So in a world without runtime
, we can only keep around the APIs where you bring your own connection.
For tests, let me look into adding CI logic to test at least compilation on WASM. |
9ba424d
to
5a71bca
Compare
In theory, the new CI config should test compilation on WASM! |
d52205e
to
ff8a387
Compare
I just removed the [target.'cfg(not(target_arch = "wasm32"))'.dependencies]
tokio-postgres = "0.7.9"
[target.'cfg(target_arch = "wasm32")'.dependencies]
tokio-postgres = { version = "0.7.9", default-features = false } Could you please check if that works for you? This should be a non breaking release for non-WASM targets now. |
@bikeshedder oh actually I think this doesn't work, due to rust-lang/cargo#1197 In our downstream crates we are seeing the default features pulled in when compiling on WASM. |
Currently,
deadpool-postgres
only offers APIs for creating pools that instantiate their underlying connections based on atokio_postgres::Config
. In some situations (mocking, custom networking), it is useful to provide an alternate connection mechanism. Thankfully,deadpool-postgres
already has theClient
trait for this so we just need to expose it publicly. As part of this, we also expose the existingConnectImpl
as a publicConfigConnectImpl
for code that needs to abstract over a selection of connection strategy.Our main use case for this is to enable use on WASM, where
tokio_postgres
is already supported but does not have thetokio-postgres/runtime
feature enabled (which enables the APIs for creation based on a database URL). So this PR also adds aruntime
feature that maps to the underlyingtokio-postgres
one for now (defaulting to true to preserve compatibility). It also includes a minor patch to disable thekeepalives_idle
config on WASM where it is not supported.