-
-
Notifications
You must be signed in to change notification settings - Fork 89
Add support for wasm32-unknown-unknown
target
#232
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
base: main
Are you sure you want to change the base?
Conversation
tokio_postgres::Socket is not supported for wasm32-unknown-unknown target. tokio_postgres, additionally, may return a Connection type whose first bound is not a tokio_postgres::Socket. Accordingly, let's relax the bounds on try_from_client_and_connection so that we match the loosest type that may be returned by tokio_postgres's connection methods, which enables try_from_client_and_connection to work for a wasm32-unknown-unknown 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.
Thanks for opening this PR. Generally speaking I'm open to support this.
That written, there are a few things that I would like to clarify/improve before merging.
- The inline comments
- Changing the cfgs to rather use something like
#[cfg(target_family = "wasm")]
instead to also support other wasm targets - Adding some test/example
- Adding some sort of CI check that this works. That should at least check if it compiles, ideally run all tests. Maybe Add wasm32-unknown-unknown target support for sqlite diesel-rs/diesel#4411 could serve as an example for the testing?
#[cfg(not(target_arch = "wasm32"))] | ||
fn establish(database_url: &str) -> impl Future<Output = ConnectionResult<Self>> + Send; |
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 would like to keep this method in all configuration.
It should be possible to provide an implementation for this for wasm32
as you somehow need to establish a connection there as well, right?
#[cfg(target_arch = "wasm32")] | ||
wasm_bindgen_futures::spawn_local(async move { | ||
match futures_util::future::select(shutdown_rx, conn).await { | ||
Either::Left(_) | Either::Right((Ok(_), _)) => {} | ||
Either::Right((Err(e), _)) => { | ||
let _ = error_tx.send(Arc::new(e)); | ||
} | ||
} | ||
}); | ||
|
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.
Given that the only difference here to the tokio::spawn
call above is the name of the spawn function we maybe could refactor this to something like that to minimize code duplication:
let background_future = async move {
match futures_util::future::select(shutdown_rx, conn).await {
Either::Left(_) | Either::Right((Ok(_), _)) => {}
Either::Right((Err(e), _)) => {
let _ = error_tx.send(Arc::new(e));
}
}
};
#[cfg(not(target_arch = "wasm32"))]
tokio::spawn(background_future);
#[cfg(target_arch = "wasm32")]
wasm_bindgen_futures::spawn_local(background_future);
tokio-postgres
includes support for targetingwasm32-unknown-unknown
, and it would be wonderful to have ORM functionality with Diesel in WASM environments.This PR enables
diesel-async
compilation with thepostgres
feature for thewasm32-unknown-unknown
target. Please note that I have not explored what is required to supportwasm32-unknown-unknown
for other database or connection types.I am currently using this within a Cloudflare Workers deployment. (As an aside to anyone exploring, I have not yet been able to configure
diesel-async
to work with Hyperdrive, presumably due to some Postgres feature set thatdiesel-async
leverages and that Hyperdrive is not currently supporting. Usingdiesel-async
with a direct connection string without Hyperdrive, however, works as expected.)