Enable JSON1 extension for SQLite

jeremyBanks commented 3 years ago

This was previously declined in #20, but I would like to encourage you to reconsider. 🙂

I am currently using Node with the NPM sqlite3 package for a small project, because it supports the JSON functions out-of-the-box. (I would have needed to do a custom build to enable them for Python or Ruby's primary SQLite libraries, and that seemed daunting.) I would have liked to have chosen Deno instead, but this library didn't support them.

I use SQLite as an in-memory database to provide indexing and data validation/integrity guarantees. Like many JavaScript developers, the data I work with often comes in the form of loosely-structured JSON objects, for better or worse. I find it extremely valuable to be declare constraints and indices against JSON properties, gradually-typing my data in a TypeScripty fashion while having SQLite confirm my assumptions against the real data. It's nice to be able to have all of this validation logic expressed together in SQL, instead of needing to have one set of logic (or transformations) in JavaScript for the JSON objects (maybe using io-js or runtypes), and a separate set of validation logic in SQL.

Fortunately, it seems like enabling this extension statically doesn't require compiling any of the dynamic extension support, so the impact on build complexity and size is small: it just requires adding one build flag, and the compiled WASM is only 3% larger. Given the prevalence of JSON data in JavaScript, I suggest this capability may be worth that cost.

sqlite.wasm:    621987B ➜ 642364B (+3.2%)
sqlite.js:      829955B ➜ 857127B (+3.2%)
gzip sqlite.js: 339540B ➜ 349630B (+3.0%)

Thank you for your work on this library.

(The random style changes in here were made by deno fmt, in order to satisfy the CI.)

dyedgreen commented 3 years ago

Hi @jeremyBanks, thank you so much for submitting this pr, and apologies for taking so long to get back to you.

I'm currently quite busy, with exams coming up, but I just had some time to fix one of the current issues and wanted to update you on this issue:

Regarding the added binary size, I'm not too concerned with this, since the change is quite small and this is a server side library. My main issue was with added complexity / maintenance burden, but looking at the PR, this seems a matter of adding a simple build flag, so that should actually also be fine.

I'll give your change a proper code-review later, but from having a quick look, here is some things I'd like to change before merging this PR 😅

You updated the dev container, this should probably be a separate PR
You're changing the compiled binary, which I'd prefer to automate

The build stuff is not your fault, but in short I would want to add this as a CI step before accepting the PR, so that users of the library don't have to trust that the binary is not malicious in some way (which the CI let's them verify). This is mostly fine since Deno is sand-boxed, but adding a little extra verify-ability never hurts. Plus it makes contributing C changes easier, since you can't accidentally commit a debug build to master. (To be clear, this is also an issue with the binary built locally by myself, it's just now surfaced again when looking through this PR 😄)

So in short: Please bear with me to upgrade the CI (or feel free to submit a PR that does that 😉)

jeremyBanks commented 3 years ago

@dyedgreen No worries, I appreciate you taking the time to update me. You raise good points; I do feel a little weird committing a binary I compiled locally. 🙂 When I have some time I'll update the PR, and possibly take a stab at the CI.

dyedgreen commented 3 years ago

This looks good to me! Can you rebase on master, so I can merge it? 😃

jeremyBanks commented 3 years ago

Feel free to squash if you want, I know these PRs have become pretty messy, up to you.

dyedgreen commented 3 years ago

Actually, can you build the WASM again? (I think you deleted the files, and it's causing the tests to fail 😅). Then I'll squash and merge 👍

dyedgreen commented 3 years ago

Alright, if you rebase on master it should go through now? 😃

jeremyBanks commented 3 years ago

I think that the WASM from master would still fail because of our new test for JSON functions. (edit: Oh! I see that your changes would avoid that, cool! Will do.)

Attempting to rebuild at https://github.com/jeremyBanks/deno-sqlite/runs/1730010409

jeremyBanks commented 3 years ago

@dyedgreen I think we're good. :) Your approach should prevent the need for re-building before merging to master.

This PR's workflow changes make it possible to do so manually, as I have done here at https://github.com/jeremyBanks/deno-sqlite/actions/runs/496785163, but feel free to revert the workflow changes here if you think it's out of scope or unnecessary. (You should have write access to this PR branch, if you feel like doing that.)

dyedgreen commented 3 years ago

This PR's workflow changes make it possible to do so manually, as I have done here at https://github.com/jeremyBanks/deno-sqlite/actions/runs/496785163, but feel free to revert the workflow changes here if you think it's out of scope or unnecessary. (You should have write access to this PR branch, if you feel like doing that.)

I reverted the changes to the workflow, since running it manually should not be necessary. (The test runner should work without any tampering 😅 )

dyedgreen commented 3 years ago

Thanks again for the PR @jeremyBanks! 😍

dyedgreen / deno-sqlite

Enable JSON1 extension for SQLite #88