Skip to content
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

D1 beta support #329

Merged
merged 17 commits into from Sep 16, 2022
Merged

D1 beta support #329

merged 17 commits into from Sep 16, 2022

Conversation

geelen
Copy link
Contributor

@geelen geelen commented Aug 4, 2022

This adds @miniflare/d1 package:

  • Statement class, acts similarly to https://github.com/WiseLibs/better-sqlite3/blob/master/docs/api.md#class-statement but is entirely lazy, only truly gets prepared & bound when .all(), .first(), .run() or .raw() are called.
  • BetaDatabase class, acts as a D1 binding within Miniflare
  • createSQLiteDB returns a better-sqlite3 DB instance for manipulating an in-memory or on-disk Sqlite3 database. Used in Wrangler for running commands against local DBs.
  • getSQLiteNativeBindingLocation returns the location of the better_sqlite3.node file, which may not be installed in the current project.

Note this uses npx-import (https://npx-import.pages.dev) to lazily load better-sqlite3, otherwise every installation of miniflare would add ~30s to installation (including npx wrangler ...)

Aside: You can run time npx -y -p better-sqlite3 echo to see how long it takes on your machine:

npx -y -p better-sqlite3@~7.6.2 echo  21.77s user 0.69s system 92% cpu 24.276 total

On a M1 Pro ^

@cloudflare-pages
Copy link

cloudflare-pages bot commented Aug 4, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: fc1d136
Status: ✅  Deploy successful!
Preview URL: https://bfd3b66b.cf-miniflare.pages.dev
Branch Preview URL: https://d1-beta-support.cf-miniflare.pages.dev

View logs

@geelen geelen force-pushed the d1-beta-support branch 2 times, most recently from a788cc1 to 3ea70af Compare August 12, 2022 17:25
@geelen geelen force-pushed the d1-beta-support branch 2 times, most recently from 63cc329 to 14402c9 Compare September 12, 2022 12:21
@geelen geelen marked this pull request as ready for review September 12, 2022 12:32
Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 🎉 Added a few comments, mostly around hiding internal methods from users' workers.

packages/d1/README.md Outdated Show resolved Hide resolved
packages/d1/src/database.ts Outdated Show resolved Hide resolved
packages/d1/src/database.ts Outdated Show resolved Hide resolved
packages/d1/src/database.ts Outdated Show resolved Hide resolved
packages/d1/src/database.ts Outdated Show resolved Hide resolved
packages/d1/src/database.ts Outdated Show resolved Hide resolved
packages/d1/src/database.ts Outdated Show resolved Hide resolved
packages/d1/src/plugin.ts Outdated Show resolved Hide resolved
packages/storage-memory/src/local.ts Outdated Show resolved Hide resolved
packages/miniflare/src/api.ts Show resolved Hide resolved
@mrbbot
Copy link
Contributor

mrbbot commented Sep 13, 2022

Closes #277

@CraigglesO
Copy link
Contributor

You may have already fixed this. Browsing the code I couldn't tell. I made a duplicate of this branch for myself just for testing purposes because if you use memory (and possibly persist) in testing:

  1. beforeEach function adds data to DB
  2. normal test fetches a worker that looks up said data

What happens is the data is wiped because the plugin asks for the DB again... but an in memory DB would just create a new one, and I think sometimes there are issues even with a persist db.

My patch works but isn't great just to show you how I fixed it. But I figured it would be something good to know before this is merged.

@geelen
Copy link
Contributor Author

geelen commented Sep 14, 2022

That's a good point, @CraigglesO... Each test should start from a clean slate (if you're using in-memory DBs, at least), but beforeEach is really part of a test, so it shouldn't get wiped between.

Does the same problem occur with KV or R2? I wonder if it's something boneheaded I've done with the D1 stuff or a problem more generally...

@CraigglesO
Copy link
Contributor

So KV and R2 pull data in from the storage class.
For instance memory storage just keeps all key-values in a map. BUT, once a storage is created, it's stored in a StorageFactory as a top level plugin in Miniflare, so it won't be dumped as long as the miniflare instance exists (or disposed).

Currently in the case of memory (and I'd argue for persistence as well), it is creating a brand new better-sqlite instance everytime it is fetched.

If you cache the instance for later fetches that should work fine. You can look to DurableObjects for how to cache and cleanup on each reload, and on dispose.

No worries, this project has become somewhat monolithic. I had a terrible first PR but this one looks great otherwise.

@mrbbot - correct me if I'm wrong here please.

Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! ✅ Couple tiny things then let's get this released 🎉 I'm going to start writing the changelog/docs for 2.9.0 now. 👍

```js
import { BetaDatabase } from "@miniflare/d1";
import { MemoryStorage } from "@miniflare/storage-memory";
const db = new BetaDatabase(new MemoryStorage());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BetaDatabase doesn't take a Storage anymore. 🙁

Comment on lines +15 to +26
await db.fetch("/execute", {
method: "POST",
body: JSON.stringify({
sql: `CREATE TABLE my_table (cid INTEGER PRIMARY KEY, name TEXT NOT NULL);`,
}),
});
const response = await db.fetch("/query", {
method: "POST",
body: JSON.stringify({
sql: `SELECT * FROM sqlite_schema`,
}),
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BetaDatabase doesn't actually support these functions anymore right?

@mrbbot mrbbot merged commit 6bace6b into master Sep 16, 2022
@mrbbot mrbbot deleted the d1-beta-support branch January 23, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants