SQLite #212
SQLite #212
Conversation
src/sqlite.js
Outdated
| } | ||
|
|
||
| async function table(data, options) { | ||
| const Inputs = await requireDefault("@observablehq/[email protected]/dist/inputs.umd.min.js"); |
There was a problem hiding this comment.
I’m not wild about duplicating the pinned version here (ideally, it’d be managed in yarn.lock) and also it would be better to use the Library’s require rather than requireDefault. We have the same issue with fileAttachment.csv, so I figured this would be acceptable as a starting point, but at some point we have to figure out how to plumb the Library’s custom require through the FileAttachment function and then into the SQLiteDatabaseClient.
| return sql({locateFile: file => `https://cdn.jsdelivr.net/npm/[email protected]/dist/${file}`}); | ||
| } | ||
|
|
||
| export class SQLiteDatabaseClient { |
There was a problem hiding this comment.
Is it going to be odd that this database client doesn't extend the worker's DatabaseClient function, prototype-chain-wise, even though it implements the same interface? It only occurred to me because queryRow matches exactly the worker's implementation and could be re-used here without copying.
I was thinking alternatively, this implementation could move to the worker and it could accept a file attachment for the constructor: DatabaseClient(FileAttachment("db.sqlite")). Though, there's a lot of assumption going into that expression since nothing references "sqlite" statically.
There was a problem hiding this comment.
DatabaseClient isn’t currently part of the standard library, so I’ll need to move parts of it up here to make that work. I’ll take a crack at it.
There was a problem hiding this comment.
I thought about some more and I don’t think this is worth the effort. It’s not easy to move the DatabaseClient class into the standard library because it hard-codes the known implementations, all of which currently require coordination with the editor (since they are remote databases that require a database connector, not an in-memory local database like SQLite).
src/sqlite.js
Outdated
|
|
||
| async function table(data, options) { | ||
| const Inputs = await requireDefault("@observablehq/[email protected]/dist/inputs.umd.min.js"); | ||
| return Inputs.table(data, options); |
There was a problem hiding this comment.
We should switch out https://github.com/observablehq/observablehq/blob/f020251a0bc3a4cfd34e63a6684a41f0eadb7e6e/notebook-next/src/worker/databaseClient.js#L192 to use Inputs.Table too? or, did you already do that in the other PR?
There was a problem hiding this comment.
I didn’t yet, but I can.
There was a problem hiding this comment.
I decided to go the other way and port the current DatabaseClient implementation into the standard library (i.e., generate a basic HTML table rather than use Inputs.table). That’s at least consistent. I suppose we could still expose an abstract DatabaseClient here in the standard library to share some code with the other implementations, but I don’t think it’s urgent that we do this so I’d rather not, for now.
This adds SQL.js to the recommended libraries, exposed as
SQLite. In addition, it adds a fileAttachment.sqlite() method which returns a SQLiteDatabaseClient: an implementation of the Observable DatabaseClient interface backed by SQLite.