npm packaging for node and web #657
Conversation
There was a problem hiding this comment.
I need to add these back - but did the limbo version of these tests even work? I could not get them to run even before I made changes. Easily could've been a skill issue.
There was a problem hiding this comment.
Added back and they work - better than before.
There was a problem hiding this comment.
This is here to test the npm package as its packaged.
|
Thanks a lot! I have a few questions/issues: Single vs multiple package Doesn't this in web/package.json: now mean that then adding in and running: Issues with the package itself Also you'll notice that this doesn't quite work since the web package json has this configuration:
There's also vite-plugin-wasm-pack that could obviate the need for a separate script and you could just build the entire web package using EDIT: I quickly tested this and it should work, but it requires some changes to the vite config. e.g. We should aim so that it creates a single js bundle for the main thread and a single bundle for the worker thread, I think. (or two bundles if the worker requires another worker, but generally the module should import each other hierarchically)
By making assuming that is going to be the main entrypoint of our program, and we don't want to have the main entrypoint be a higher level interface that doesn't require postMessage and so on. Other notes We should probably consider such a higher level interface, I think. We can start with this, but eventually we'd probably like to have something like: not meant as an exact api design, but just abstract the webworker communication out. A couple of other side notes for the future:
|
|
Ok! I'll take a look at at addressing those comments. FYI it will probably be Tuesday before I get back around to this. |
|
Hey Elijah, I think we can do full ESM for both Node and web. However, we would need to drop wasm-pack and use wasm-bindgen and wasm-opt directly. wasm-pack would just be used one time to bootstrap the required files like package.json and from there we handle those manually in a build script. It shouldn't be complicated, unless massive changes are introduced to the bindings. What do you think about that? Let me set up a PoC. I'll send a PR when I prove that it works for web and Node, including different frontend setups that use different bundlers like Webpack (Next) and Rollup (Vite). If it doesn't work, I'll let you know. You could rebase on top of that PR or just take whatever from it and close it. |
|
I have done a full reorg and it is a single package.
I agree - I was just planning on tackling that in another PR. |
Update build script to build both Update package.json Add basic test of node variant of npm package.
Node one broke
going to try out something different
src moved under web/ to make it cleaner build does less moving of files, mostly just moves the wasm-pack into dist for node and web
run via npm run test -w web
Handle commonjs and esm module entry points tests works
elijahmorg
force-pushed
the
elijah/npm-packaging2
branch
from
b15971c to
6a7b269
Compare
|
I also have an idea for how to drop needing limbo-worker.js but maybe bootstrap the vfs and stuff directly IN lib.rs. I will have to play with it and see if it has any true merit. That way the worker wrapper can truly be for message passing between the main thread and a limbo thread. As a side note I should be around for the weekend to keep working on this at a more rapid pace :) |
|
@LtdJorge have you made any progress? Do you have any thoughts about this PR? |
|
I quickly tested diff --git a/bindings/wasm/test-limbo-pkg/package.json b/bindings/wasm/test-limbo-pkg/package.json
index 19b752a..ecffb41 100644
--- a/bindings/wasm/test-limbo-pkg/package.json
+++ b/bindings/wasm/test-limbo-pkg/package.json
@@ -3,7 +3,7 @@
"private": true,
"type": "module",
"dependencies": {
- "limbo-wasm": "file:../limbo-wasm-0.0.11.tgz"
+ "limbo-wasm": "[email protected]"
},
"scripts": {
"dev": "vite"
diff --git a/bindings/wasm/test-limbo-pkg/vite.config.js b/bindings/wasm/test-limbo-pkg/vite.config.js
index 1787468..4cb9b28 100644
--- a/bindings/wasm/test-limbo-pkg/vite.config.js
+++ b/bindings/wasm/test-limbo-pkg/vite.config.js
@@ -9,6 +9,9 @@ export default defineConfig({
"Cross-Origin-Opener-Policy": "same-origin",
"Cross-Origin-Resource-Policy": "cross-origin",
},
+ fs: {
+ allow: ["../web/dist"],
+ },
},
worker: {
format: "es",And now it works on web. Good stuff! However the node example in diff --git a/bindings/wasm/examples/example.js b/bindings/wasm/examples/example.js
index a33d08b..b6f5351 100644
--- a/bindings/wasm/examples/example.js
+++ b/bindings/wasm/examples/example.js
@@ -2,6 +2,10 @@ import { Database } from 'limbo-wasm';
const db = new Database('hello.db');
+db.exec('CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)');
+
+db.exec('INSERT INTO users (name) VALUES (\'Alice\')');
+
const stmt = db.prepare('SELECT * FROM users');
const users = stmt.all();
diff --git a/bindings/wasm/examples/package.json b/bindings/wasm/examples/package.json
index a76c298..551dd78 100644
const users = stmt.all();
diff --git a/bindings/wasm/examples/package.json b/bindings/wasm/examples/package.json
index a76c298..551dd78 100644
--- a/bindings/wasm/examples/package.json
+++ b/bindings/wasm/examples/package.json
@@ -13,6 +13,6 @@
"dependencies": {
"better-sqlite3": "^11.5.0",
"drizzle-orm": "^0.36.3",
- "limbo-wasm": "../pkg"
+ "limbo-wasm": "[email protected]"
}
}
diff --git a/bindings/wasm/package.json b/bindings/wasm/package.json
index e450385..15143d5 100644
--- a/bindings/wasm/package.json
+++ b/bindings/wasm/package.json
@@ -14,8 +14,9 @@
"module": "./web/dist/index.js",
"exports": {
".": {
- "require": "./node/dist/index.cjs",
- "import": "./web/dist/index.js"
+ "node": "./node/dist/index.cjs",
+ "browser": "./web/dist/index.js",
+ "default": "./web/dist/index.js"
},
"./limbo-worker.js": "./web/dist/limbo-worker.js"
}, |
|
EDIT: I've fixed this - but wanted to explain what the original bug in the integration-tests was. I was working on adding back in the integration tests and realized they never worked (at least the way they were thought to). If you checkout You need to delete the hello.db file between runs or else it uses the hello.db from the better-sqlite3 run. So modify connect function to delete it and you will see the tests actually error out. Another way to test it would be just run the limbo test without the node test. const connect = async (path_opt) => {
unlinkSync("hello.db");The errors are This is because the exec function doesn't contain anything. I changed that in #594 (here)[https://github.com//pull/594/files#diff-3901e74bdac2cf162afe31b5b6392917952c241644a9fe77876ce90d2226dfa6R50] by adding a body to The tests break in a different way now - but they were broken before (just hidden) and broken now. I will add them back here, but they are broken. |
|
@jussisaurio Wow! Great feedback and detailed, thanks! I'll make sure the examples work next! |
|
RE: this #657 (comment) the current error if you don't clean up the file is I am fixing this by removing the drop table for the time being. |
Fix package.json for better imports vs web/nodejs Fix examples and integration tests
|
This should be good to go in now (unless someone has more comments!). |
|
@elijahmorg If people have more comments, let's address them in-tree. Merged, thanks a lot for doing this! 👏👏👏 |
Add web support to npm package.
Still a WIP need to do some cleanup still
I assumed it is better to keep the server code and web code together in the same package (bigger download number). It took quite a bit of experimentation but the ultimate experience is
node - commonjs
web - module
Like I said this took a lot of experimentation on my part as this is my first time trying to create an npm package let alone that mixes commonjs and modules.
The structure is an npm workspace with two sub packages (web and node).
The output of wasm-pack gets put in <web/node>dist
JS code moves into <web/node>src/
Tests move under web/test
The npm package looks like (you can see I need to cleanup some of the stuff that gets included).
resolves #624