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

Add IDBFS support #397

Closed
wants to merge 1 commit into from
Closed

Add IDBFS support #397

wants to merge 1 commit into from

Conversation

kegsay
Copy link

@kegsay kegsay commented May 22, 2020

This PR adds support for IDBFS by:

  • Enabling IDBFS support at compile time.
  • Exporting the filesystem objects at runtime so things like FS.syncfs can be called.
  • Modifying the Database API to accept a string in addition to a Uint8Array in its constructor, which sets this.filename accordingly. This is to allow for backwards compatibility. Previously, passing strings would throw.

There's one major shortcoming with this PR which is that I have to remove --closure 1 on EMFLAGS_OPTIMIZED otherwise some methods on FS are minified (mount and syncfs are affected). I'm unsure why the closure compiler is doing this.

To use IDBFS, users need to:

const initSqlJs = require('sql.js');
const SQL = await initSqlJs();

SQL.FS.mkdir("/idb");
SQL.FS.mount(SQL.IDBFS, {}, "/idb");
// load what was previously stored in IndexedDB
// wait for syncfs callback to be invoked to know when it is ready.
SQL.FS.syncfs(true, function(err) { .... });
// uses existing content or a new file is made
const db = new SQL.Database("/idb/myfilename.db");

// ... do some work ...

// save to IndexedDB, wait for callback to be invoked to know when it is complete
SQL.FS.syncfs(false, function(err) { .... });

IDBFS support is preferable in some cases over Database.export as that function will close the database/clear prepared statements and make the Database subsequently unusable. SQL.FS.syncfs however will not close the database.

Fixes #302 but for clarity: this is just writing files, so is database-level scoped, not table/row level scoped.

@lovasoa
Copy link
Member

lovasoa commented May 22, 2020

It is a great feature ! However, I'm not sure we want to re-expose the full emscripten filesystem API, and change the interface of the Database constructor. Could we more simply flush caches instead of closing the database when calling export ? This would let you use it with indexeddb, and benefit all the existing users of Database.export.

@lovasoa
Copy link
Member

lovasoa commented May 22, 2020

Also, looking at the generated assets, it looks like the changes in this pull request roughly double the size of sql-wasm.js. I understand that we need to load the huge wasm file afterwards anyway, but sql-wasm.js is likely to end up on the critical path of the final web application...

@kegsay
Copy link
Author

kegsay commented May 22, 2020

The size increase will be mostly due to closure compiler's absence. Flushing caches may work, but I worry it would race between flush -> export, but then again I think we do all of that on one event loop tick so it might be okay if that's the case.

@lovasoa
Copy link
Member

lovasoa commented May 22, 2020

The size increase will be mostly due to closure compiler's absence. Flushing caches may work, but I worry it would race between flush -> export, but then again I think we do all of that on one event loop tick so it might be okay if that's the case.

The code is completely synchronous, no race is possible.

@seidtgeist seidtgeist mentioned this pull request May 28, 2020
3 tasks
@kegsay
Copy link
Author

kegsay commented May 29, 2020

Okay, it sounds like you don't want this feature to be done in this way, so closing this.

@kegsay kegsay closed this May 29, 2020
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.

Anyway to save SQL DB to IndexedDB without calling export() for persistence?
2 participants