-
Notifications
You must be signed in to change notification settings - Fork 223
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
Add wasm
module & cloudflare client
#571
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.
I'll be taking another look today or tomorrow since there are lot of changes, looks reasonable so far. In a follow-up, we should also just remove all occurrences of the word "hrana" from libsql too, and stick to naming it "remote"
@@ -7,6 +7,6 @@ edition = "2021" | |||
crate-type = ["cdylib"] | |||
|
|||
[dependencies] | |||
js-sys = "0.3.64" |
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.
nitpick: Any reason for that change? It essentially just means ">= 0.3.64", so it's a little stronger than "0.3", but still makes sense.
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.
Agreed, better to be specific here as long as we don't do =
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.
Cloudflare worker crate is pinned to specific version which was different at patch level.
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 fine for now cargo will pick the latest version anyways and I don't believe we actually use code that requires a higher version but if somehow we do and a user runs into that also somehow we can fix it in a non-breaking way.
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.
Overall, really nice work! I left a few comments and if you have any questions don't hesitate to ping me on slack to discuss.
So at a high level, I think the trait approach is correct based on what we discussed but there are a couple of things I think we want to revise before we merge.
- I think we want to avoid the extra crate and in-turn avoid the duplication of the code.
- We want to expose less public structs and fn. We should only need the most minimal. I think even for the first iteration we can skip on stuff like
statement()
/transaction()
. - The structure within
libsql
, I think the new code which imo we should callserverless
should live within apub mod serverless
. This means that most users will not see duplicate connection types in the root of the docs but instead will be directed to aserverless
module that contiains specific docs and connection types in there.
I am imagining that a user could use it like so:
use libsql::serverless;
let conn = serverless::Connection::open_cloudflare_workers(); // This would return `serverless::Connection<CloudflareWorkers>`
conn.execute("foo bar", ()).await?;
conn.query("foo bar", ()).await?;
Since, we have not implemented proper execute_batch
for hrana we can leave that out for the moment.
In general, I would like to see how we can implement this with a less intrusive code change and avoid re-implementing things we already have. Let me know if you have any questions. Also, more than happy to hop on a call and talk over this.
@@ -7,6 +7,6 @@ edition = "2021" | |||
crate-type = ["cdylib"] | |||
|
|||
[dependencies] | |||
js-sys = "0.3.64" |
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.
Agreed, better to be specific here as long as we don't do =
libsql-remote/src/cloudflare.rs
Outdated
|
||
impl CloudflareSender { | ||
async fn send(url: String, auth: String, body: String) -> Result<ServerMsg> { | ||
use worker::*; |
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.
nit: I always prefer explicit imports as it makes understanding where things come from much easier without an ide (aka on github). I know workers docs use the glob import but I don't think its good style at all to ever use a glob import (there is one case and its when you don't want to reference to the prefix of an enum so you can use MyEnum::*
)
libsql-remote/src/proto.rs
Outdated
Value::Float { value } => write!(f, "{}", value), | ||
Value::Text { value } => write!(f, "{}", value), | ||
Value::Blob { value } => { | ||
write!(f, "{}", STANDARD_NO_PAD.encode(value)) |
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.
Whats the motivation for this 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.
My bad, should be other way around.
34cbcda
to
734f2b5
Compare
serverless
module & cloudflare client
cb7d7cb
to
8024e08
Compare
serverless
module & cloudflare clientwasm
module & cloudflare client
ce3127a
to
88cd3e4
Compare
88cd3e4
to
9d3d335
Compare
This PR extracts Hrana protocol into a separate library (libsql-remote) for the purpose of being able to create a libsql remote connection in different environments - which can possibly have very narrow requirements like ie. Cloudflare workers.
libsql-remote
is a dependency that is used bylibsql
with (feature = "hrana") turned on.libsql-remote
uses a remoteHttpSend
abstraction that can be implemented to provide an ability to send HTTP requests - this is implemented inlibsql-remote
itself for Cloudflare (requires feature = "cloudflare"). Another implementation is hyper HttpSender, that exists inlibsql
itself.libsql-remote
has an API that's slightly different from general-purposelibsql::Database
- the main reason is that we don't take advantage from using pooling here - it's implemented bylibsqld
itself and I don't think it's responsibility of a Hrana connection to provide such implementation.Cloudflare Example