Update to latest Neon #103
Conversation
kjvalencik
force-pushed
the
kv/update-neon
branch
from
4a2c3fd to
0ab95a0
Compare
kjvalencik
force-pushed
the
kv/update-neon
branch
2 times, most recently
from
eb82a56 to
ee65857
Compare
kjvalencik
force-pushed
the
kv/update-neon
branch
from
ee65857 to
67b4bc7
Compare
dherman
left a comment
dherman
left a comment
There was a problem hiding this comment.
There's so much amazing simplification here! The one that took me the most work to understand was the async-sqlite example. I don't have major code changes to suggest, but I do think as an example repo it would benefit from more explicit explanations of how things wire up together. I left a bunch of suggestions.
| // Immediately close the connection, even if there are pending messages | ||
| DbMessage::Close => break, | ||
| while let Ok((d, callback)) = rx.recv() { | ||
| // Returning `true` means to _stop_ |
|
|
||
| Ok(()) | ||
| // Idiomatic rust would take an owned `self` to prevent use after close | ||
| // However, it's not possible to prevent JavaScript from continuing to hold a closed database |
There was a problem hiding this comment.
What happens if they call methods on a closed database? Should we wrap tx in an Option?
There was a problem hiding this comment.
OK I think I get how this works, but it's incredibly subtle. Check my understanding:
- When an operation fails or the user calls
.close()for the first time, the operation producesControlFlow::Break() - This causes the database thread's main loop to terminate, and the thread terminates
- The thread owns the mpsc receiver, so it gets dropped
- Subsequent attempts to use the sender will fail because the mpsc semantics is that when one half is dropped, most operations on the other half fail from that point on
It would be helpful to explain at least some of this in the comments.
| V: Value, | ||
| { | ||
| let (d, promise) = cx.promise(); | ||
| let Err(mpsc::SendError((d, _))) = self.tx.send(( |
There was a problem hiding this comment.
This let-else-return followed by d.reject() is some somewhat confusing control flow. Not sure if you tried alternatives but I'd be tempted to use an old-fashioned match. Your call though.
|
|
||
| type DbCallback = Box<dyn FnOnce(&mut Connection, &Channel, Deferred) + Send>; | ||
| type DbCallback = | ||
| Box<dyn FnOnce(&mut Connection, &Channel, Deferred) -> ControlFlow<(), ()> + Send>; |
There was a problem hiding this comment.
This is a beast. It needs a comment explaining its purpose. There are so many callbacks in this demo, it's hard to understand the purpose of each.
There was a problem hiding this comment.
OK I think I understand it now:
- The SQLite database lives on a separate database thread.
- INSERT and SELECT operations are wrapped up as thunks and sent to that thread.
- The high level version of a database operation thunk is just a function from a SQLite Connection to a TryIntoJs value
- DbCallback is the lower level version, which needs the deferred and the channel to send its result value to, and returns a ControlFlow to indicate whether to kill the database thread
| type DbCallback = | ||
| Box<dyn FnOnce(&mut Connection, &Channel, Deferred) -> ControlFlow<(), ()> + Send>; | ||
|
|
||
| // Wraps a SQLite connection a channel, allowing concurrent access |
There was a problem hiding this comment.
Not quite grammatical. Also it's subtle that the SQLite connection is hiding in this type because it's implicit in the thread closure that the channel is connected to. That's worth explaining here.
| } | ||
|
|
||
| // Inserts a `name` into the database | ||
| // Accepts a `name` and returns a `Promise` |
There was a problem hiding this comment.
Say a Promise<number> here to make it clearer that the rowid is the result value of the promise.
| Ok(cx.number(id as f64)) | ||
| }); | ||
| fn insert<'cx>(&self, cx: &mut Cx<'cx>, name: String) -> JsResult<'cx, JsPromise> { | ||
| self.exec(cx, move |conn| -> Result<_, Error> { |
There was a problem hiding this comment.
Make the f64 explicit so the reader doesn't have to do the inference in their head.
No description provided.