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

[feedback wanted] Removing the webworker code from sql.js #377

Open
lovasoa opened this issue Mar 31, 2020 · 26 comments
Open

[feedback wanted] Removing the webworker code from sql.js #377

lovasoa opened this issue Mar 31, 2020 · 26 comments
Labels
discussion Not an issue

Comments

@lovasoa
Copy link
Member

lovasoa commented Mar 31, 2020

I feel like today, providing a custom, limited webworker API, based on message-passing instead of promises is not the best choice, and is not useful to many. On the contrary, it creates confusion to have so many different assets available for download. When needed, it is very easy to turn the main library code into a web worker, in an automated way, without loosing any of the original API, using libraries like comlink.

What I propose :

  • remove the worker-specific code from the main sql.js repository
  • add a note in the readme about how to run the code in a web worker
  • if someone can commit to maintaining the old WebWorker code (@kaizhu256 ?) : create a second repository in the sql-js organization, containing the webworker, publishing separate release assets. The repository could use continuous integration to automatically build assets without duplicating any code from the main sql.js repository. I can create the initial setup for the repository if someone is willing to maintain it in the long term.
@kaizhu256
Copy link
Member

kaizhu256 commented Mar 31, 2020

i have an idea to provide a minimal-maintennance promise-based worker in ~100 lines of code in src/api.js. will provide a pull-request of it to see what you think. been using my simple-hack for awhile in production. its usage is as follows:

<script src="dist/sql-wasm.js"></script>
<script>
// example-usage
var sql = await initSqlJs();
// src/api.js now has a new Worker class (~100 lines of extra code)
// with one promise-method Worker.prototype.postMessage
var worker = new sql.Worker(
    "https://foo.com/dist/worker.sql-wasm.js"
);
try {
    var result = await worker.postMessage({
        action: "exec",
        sql: "SELECT * FROM myTable"
    });
} catch (sqlError) {
    console.error(sqlError);
}
/*
result = {
    messageId: 1,
    results: [...]
};
 */
</script>

@brodybits
Copy link
Contributor

it is very easy to turn the main library code into a web worker [...] using libraries like comlink.

comlink definitely looks nice, maybe add an example to the documentation?

minimal-maintennance promise-based worker in ~100 lines of code in src/api.js

Definitely a nice idea. I wonder if we could put this in a separate module in this org and give a pointer. I am personally biased towards packages in small, focused modules whenever possible https://blog.sindresorhus.com/small-focused-modules-9238d977a92a

@lovasoa
Copy link
Member Author

lovasoa commented Mar 31, 2020

@kaizhu256 Let's try to stay focused. What do you think about separating the worker code from the main library ? If you have your separate library, you can then make the changes you want to it.

@kaizhu256
Copy link
Member

admit i'm biased against splitting worker-code into separate module.

as product-developer, i've had years of poor-experience translating modular-based design-patterns into deliverable UX-features. and i feel like web-workers is integral to sql.js.

workers are mandatory for common use-case of manipulating data > 20mb in sql.js (and still have satisfactory UX). so i feel strongly it should be a builtin feature (if i can make it easy-to-maintain).

@lovasoa
Copy link
Member Author

lovasoa commented Mar 31, 2020

I do agree that most users of the library will want to use sql.js inside a web worker. But I don't think most want the library itself to provide the web worker. Modern javascript stacks almost always use a compiler, and people will want something that integrates well with their existing stack, not a separate file with a separate poorly-documented API.

@kaizhu256
Copy link
Member

@lovasoa how about getting rid of worker.sql-xxx.js as follows:

  1. make src/worker.js isomorphic (like - make worker.sql-wasm.js isomorphic (will load in web-worker, nodejs, and browser without errors). #376) and merge it into src/api.js

  2. for legacy-compatibility, update Makefile to alias worker.sql-xxx.js from sql-xxx.js (eventually will deprecate and remove the alias).

  3. update src/api.js with a no-frills SQL.Worker class that exposes a single promise-based method, SQL.Worker.prototype.postMessage

all-in-all we've gotten rid of maintaining the separate worker-file, and only need to document SQL.Worker.prototype.postMessage

@lovasoa
Copy link
Member Author

lovasoa commented Mar 31, 2020

The goal here was to remove the web worker, not to make it the default!
People using another web worker strategy or no web worker don't want to pay for other unrelated web worker code. I'm not sure I understand what would be the inconvenient of having your custom web worker interface distributed as a separate project. Why would it be more difficult for you to integrate and use ?

@brodybits
Copy link
Contributor

While I would favor moving the web worker code out, I wonder if we should ask ourselves what kind of an API we want to export in the first place. I can think of the following quick alternatives:

  • SQL.js exports a lower-level, synchronous API and have users use a wrapper if they want a higher-level asynchronous API that could be used more directly from a worker
  • SQL.js exports a slightly higher-level asynchronous API and would just work from a worker or a very simple wrapper
  • export both synchronous and asynchronous APIs

While I would favor the first and downvote the last, I do think all of these alternatives should be valid for discussion. For example, Node.js fs API does export both synchronous and asynchronous API variants.

@kaizhu256
Copy link
Member

because then the sqli.js online demo will depend on the external worker-github-repo. it creates more maintainability-issues for the demo.

@kaizhu256
Copy link
Member

@brodybits, yes SQL.Worker.prototype.postMessage is a low-level, no-frills async worker-api (in 100 lines of code or less). its for product-developers like me who don't want to deal with yet-another-module to get basic-functionality out of sql.js.

@lovasoa
Copy link
Member Author

lovasoa commented Mar 31, 2020

You could have a demo using your api on your repo, and there would be a different demo using a more traditional stack on the main sql.js repo. What do you think ?

@kaizhu256
Copy link
Member

so make the online-demo externally depend on experimental comlink? that sounds like more maintainnence headaches :`(

@lovasoa
Copy link
Member Author

lovasoa commented Apr 1, 2020

Why "externally" ? Comlink will be compiled and shipped with the demo. And why "experimental" ? comlink is a very small well-maintaoned library that is at version 4, has been around since 2017, and has nearly 10k weekly downloads on npm. What kind of maintenance headaches could it cause ?

@kaizhu256
Copy link
Member

kaizhu256 commented Apr 1, 2020

hmm, the comlink vanilla-js dist-file dist/umd/comlink.js is only 300 lines. can we embed/compile it directly into sql.js dist-files dist/sql-xxx.js? i still want sql.js to have builtin async-api out-of-the-box.

the async api could be built-into sql.js as follows (perhaps added to src/shell-post.js):

 // src/shell-post.js
 
 
 
         // The shell-pre.js and emcc-generated code goes above
         return Module;
     }); // The end of the promise being returned
 
   return initSqlJsPromise;
 } // The end of our initSqlJs function
 
 // This bit below is copied almost exactly from what you get when you use the MODULARIZE=1 flag with emcc
 // However, we don't want to use the emcc modularization. See shell-pre.js
 if (typeof exports === 'object' && typeof module === 'object'){
     module.exports = initSqlJs;
     // This will allow the module to be used in ES6 or CommonJS
     module.exports.default = initSqlJs;
 }
 else if (typeof define === 'function' && define['amd']) {
     define([], function() { return initSqlJs; });
 }
 else if (typeof exports === 'object'){
     exports["Module"] = initSqlJs;
 }
 
+// if worker-env, then expose async-api via comlink
+if (
+    typeof self === "object"
+    && typeof importScripts === "function"
+    && self
+    && self.importScripts === importScripts
+) {
+    initSqlJs.then(function (SQL) {
+        Comlink.expose(SQL.Database);
+    });
+}

@kaizhu256
Copy link
Member

also, i'm not familiar with javascript-proxies. is the performance ok if i tried to ingest a 100mb sql.db blob via comlink?

@Lioric
Copy link

Lioric commented Apr 5, 2020

We are using the WebWorker API of sql.js to maintain the responsivenes of the UI, additionally we are not using any build, "compiler", packer or what is the flavor of the day javascript build tool, we use an in house build system.

So the built in WebWorker support was perfect for us, not really excited to see it in process of being deprecated and thus needing to add yet another piece to the spaghetti that javascript build enviroment is.

For what it's worth, Just wanted to mention it to let you know that it is currently being used.

The only issue that I have with this, is that the reasons for its deprecation sounds more like "promises are trendy" more than a maintenance or a real implementation limitation

@lovasoa
Copy link
Member Author

lovasoa commented Apr 5, 2020

@Lioric thank you for getting back to us ! No decision has been taken yet, and we'll take your feedback into consideration.

And, just to be clear, I wanted to deprecate it because I felt like it was used by a very small minority of users, it is complex to use, and has a limited set of features compared to the main API. Not because "promises are trendy". (are they ?)

But if it appears that it in fact has a significant number of users, then we will keep it, and continue to maintain it.

@kaizhu256
Copy link
Member

though i support keeping the webworker-code, can we deprecate the worker-dist-files and merge it into main dist-files?

  • less devops headache for maintainers having to maintain/document separate worker-release-package

  • less devops headache for users having to keep track of 2 different sql.js variants

@lovasoa
Copy link
Member Author

lovasoa commented Apr 6, 2020

@kaizhu256 : see #378 (comment) . We don't want to download and parse the same js code twice.

@kaizhu256
Copy link
Member

kaizhu256 commented Apr 6, 2020

@lovasoa user doesnt download and parse it twice.

  • the user simply calls new Worker("/dist/sql-wasm.js")
    or maybe new Worker("/dist/sql-wasm.js?isWebWorker=1")
  • no need for <script src="/dist/sql-wasm.js">

@lovasoa
Copy link
Member Author

lovasoa commented Apr 6, 2020

Okay, I thought you were talking about the #378 approach again. It still means shipping code that I think will be unused most of the time. I have already updated our release page, to hopefully make things clearer at download time. If it still causes confusion, and we see a large enough proportion of users are still using the webworker API, then yes, we could merge it into the main code.

@frankier
Copy link

Well that wasn't quite as easy as advertised: https://gist.github.com/frankier/4bbc85f65ad3311ca5134fbc744db711

I had to (ab?)use transferHandlers to make Statement a proxy too. This way we get to use the full original API, but proxying individual getAsObject calls and then copying the resulting array seems to lead to a slowdown --- so we're back to using exec anyway, which is pretty much the same level as the limited WebWorker API.

@frankier
Copy link

As @rhashimoto pointed out in my gist, you have to be careful about freeing up ComLink proxies, making its safe use even less easy than advertised.

@rhashimoto
Copy link

I use sql.js in a Worker, but I'm not using the provided WebWorker API. Instead I wrote my own Proxy-based interface to the non-Worker API, with mechanics similar to Comlink. Recently I've also been experimenting with a branch using IndexedDB as a backing store via a SQLite VFS and Asyncify (which has been educational).

I like the idea of splitting out pieces of the project into separate packages. In fact, I would not separate only the WebWorker - I would trim the main module down to provide just the raw C functions and Emscripten runtime, and put all user APIs in their own Javascript-only packages.

I think that would make it easier for a developer to modify the user APIs, and perhaps eventually contribute pull requests for new features or establish alternative APIs. Using make and Emscripten is foreign to many developers, and certainly adds friction to the typical Javascript development process. If you just want to change or add Javascript, it's less scary and more efficient if all that is hidden behind a module import. And for people that do want to make changes to the WASM it's still more efficient when working on the Javascript side even if it's slightly more bother to work on two packages at once.

@Lioric
Copy link

Lioric commented Oct 21, 2021

Just to let you know that we decided to maintain our custom WASM implementation, so at least on our side, there is no holding back on the worker.js deprecation

@martin-pabst
Copy link

martin-pabst commented Jan 10, 2022

I'm currently writing an online SQL-IDE for my students (10th to 13th grade) which lets them writing and executing SQL statements inside their browser without putting load on my webserver. Your webworker version of sql.js is perfectly suited for this as even bad sql-statements (with many joins...) from my students don't stop the editor UI from working. I'd appriciate if you keep the webworker version or if you added a description how to use the non-webworker-version inside a webworker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Not an issue
Projects
None yet
Development

No branches or pull requests

7 participants