-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Comments
i have an idea to provide a minimal-maintennance promise-based worker in ~100 lines of code in <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> |
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 |
@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. |
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). |
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. |
@lovasoa how about getting rid of
all-in-all we've gotten rid of maintaining the separate worker-file, and only need to document |
The goal here was to remove the web worker, not to make it the default! |
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:
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. |
because then the sqli.js online demo will depend on the external worker-github-repo. it creates more maintainability-issues for the demo. |
@brodybits, yes |
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 ? |
so make the online-demo externally depend on experimental comlink? that sounds like more maintainnence headaches :`( |
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 ? |
hmm, the comlink vanilla-js dist-file the async api could be built-into sql.js as follows (perhaps added to // 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);
+ });
+} |
also, i'm not familiar with javascript-proxies. is the performance ok if i tried to ingest a 100mb sql.db blob via comlink? |
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 |
@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. |
though i support keeping the webworker-code, can we deprecate the worker-dist-files and merge it into main dist-files?
|
@kaizhu256 : see #378 (comment) . We don't want to download and parse the same js code twice. |
@lovasoa user doesnt download and parse it twice.
|
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. |
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 |
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. |
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. |
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 |
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. |
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 :
The text was updated successfully, but these errors were encountered: