Add support for sqlite backed durable objects. #727
Conversation
|
alt to #723 with some additional bindgen methods, a test, and a sandbox example.
|
|
Ended up adding SqlStorageValue after attempting to use my fork. Definitely improves ergonomics. |
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
This is great, thank you.
* improve ergonomics with bindings: Vec<JsValue> -> bindings: impl Into<Option<Vec<SqlStorageValue>>> * add SqlStorageValue types * enable sqlite.spec.ts
…achedQueryForTest
not sure if this is right
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Thanks so much for following up on this. Marking the TODOs for experimental features makes sense to me. I'd be happy to keep working this one through if we can get it in for the coming release.
now have warm fuzzies
|
sry. I was only iterating on |
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
This looks great. Thanks for getting the good test coverage in as well.
The test coverage looks really solid as well. Is it possible to get a test case in where the next() iterator doesn't type match through the serde type with the expected data to test the error on that?
Apart from that just needs a last cargo fmt as well it seems, then we're just about there I think!
worker/src/sql.rs
Outdated
| /// 32-bit signed integer (safe for JavaScript Number conversion) | ||
| Integer(i32), |
There was a problem hiding this comment.
Thinking about this one again, I think it would make more sense to use i64 and then have a guard when setting to determine that it is between Number.MIN_SAFE_INTEGER and Number.MAX_SAFE_INTEGER for JS. That gets us over 6 byte integers at least, which would be a big improvement over 4 byte integers.
There was a problem hiding this comment.
hobbled something together. feels a bit messy/dangerous
+1 on cloudflare/workerd#4195
There was a problem hiding this comment.
Yeah I see what you mean about this. I'd quite like to (a) know what the upgrade path to bigint would look like and be sure this isn't now inhibiting that and (b) investigate the bigint approach more.
I guess for the bigint upgrade we would want Integer(i32) along with BigInteger(i64)?
There was a problem hiding this comment.
Or, would we just have Integer(i64) where the JS API semantics are that:
- Conversion to JS will use BigInt when outside i32 range
- Conversion to JS will always use BigInt if that's supported safely
- Conversion from JS would always support either
I guess thinking this through some more the above seems absolutely fine to me from an interop perspective without needing any other special handling, so that Integer(i64) from the start is the correct end design, since it would be more painful on the bigint upgrade path to have an opt-in variant based on range.
|
Thanks again for your work on this! |
2 participants
Fixes #645