-
Notifications
You must be signed in to change notification settings - Fork 309
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
Limbo package does not load on WebContainers #697
Comments
This doesn't only affect WebContainers, it affects every installation of class Database {} into export class Database {} fixes it. I am looking at how we can make it output ESM format. |
Thanks for looking into this @LtdJorge. I think most libraries today are targeting ESM so I do wonder if we can do something to expose it in |
Yeah, I'm banging my head on it. NextJS, to add insult to injury, uses Webpack still, instead of using Vite or something else. There is experimental support for Turbopack, by running |
Ouch, perhaps switching to CommonJS is the sane choice for now... :-/ |
Give me a moment. The templates and tools from rustwasm seem a little outdated, at least for Webpack. I think we might need two published packages, but I'm not sure yet. I'll update you when I have something more concrete. |
I think I have it figured out. Tomorrow I will send a PR, and hopefully also close #271 at the same time, which was on my radar and should be easy enough. Btw, nevermind about needing two packages, we don't, and we also can do ESM for frontend and Node. |
@LtdJorge there's some ongoing work by @elijahmorg on this: #657 Might require some coordination, although if we end up merging your fix and the other one requires rebase that's fine. |
You're right. I saw his PR for the OPFS backend but not that one. I'll comment there, because I think we can keep everything in an ESM package. |
I'm open to some ideas! I'll hopefully get back to my PR this evening (had some house projects that took longer than anticipated this week). |
You can generate esm/commonjs using wasm-pack target options. See https://github.com/tursodatabase/limbo/blob/main/bindings/wasm/scripts/build#L8 and for reference. Now I don't know if the esm output works for nodejs. |
Sadly, it doesn't, but it is because of the difference in APIs, no because of ESM. ESM is fine with Node, with the experimental version. I tried it and it works. The problem lies in how the .wasm files is loaded. If you just use the web option in wasm-pack, it uses fetch() to get the file, but we're in Node, not on the frontend, so we have to use node's fs. And in the frontend there is no fs. So, I've been cooking up something, with dynamic import(), so that the module that gets loaded is selected at runtime. I've been also reworking the bundling and the build script, to make it more straightforward. Il'll try to finish it tomorrow morning (I'm UTC+1) and send a PR for you guys to check. We can pick and choose then. |
Interesting. I think my 2 cents (but I am not an expert in this area) is that while commonjs is the lingua franca of nodejs limbo-wasm should use that and once wasm-pack supports node esm then it could convert to that. We can have the npm package work for web and nodejs and then for deno publish to the deno package registry. |
My implementation is getting a bit complex, and I don't like it. So I think we should just review and merge your PR, and if I find a better way, I'll apply some changes on top of your version later. I have been also working on making the WASM version fully asynchronous with completions based on callbacks (for Node), and adapting your OPFS implementation for it too. So I'd prefer to apply these changes to a working version of both Node bindings and web bindings, aka your PR. |
I feel that. I think you can also make it so that the wasm build exposes an asynchronous init vfs function. I was starting to work on some of that (asynchronous io), but I decided to work on packaging first. Happy to let you do that and I'll grab some other stuff. Also the db.exec stuff is very broken - it needs work as well. I discovered that the integration tests for wasm don't work (never did in reality as best I can tell). I need to open an issue for that today too. |
FYI @elijahmorg and @LtdJorge, I merged #657 and released 0.0.13. The original error is gone, but we now get this:
|
Seems like the .wasm file isn't being described in package.json or somewhere, so the bundling isn't resolving it. Or that the bundling isn't putting it in the right place. |
I was correct. I added this to import type { NextConfig } from "next";
import { type Configuration } from "webpack"
const nextConfig: NextConfig = {
webpack: (config: Configuration, options) => {
config.node = {
__dirname: true,
__filename: true,
global: true
}
return config;
},
};
export default nextConfig; And now it correctly picks up the .wasm file and the other JavaScript from import { Database } from 'limbo-wasm';
export default function Home() {
const db = new Database('/home/jorge/projects/limbo/testing/testing.db');
const stmt = db.prepare("SELECT * FROM users;");
const results = stmt.all();
return (
results.map((object, i) => {
return <div key={i} style={{paddingLeft: '6em'}}>{JSON.stringify(object)}</div>
})
)
}
|
However, having to polyfill the NodeJS variables doesn't sound like the correct way to me. I believe for Next and others like it, the correct dependency should be the web version. I'm going to look into how to correct that. |
I'll let you keep tracking this down! |
@LtdJorge @elijahmorg btw, the creator of napi-rs says that they support Wasm out of the box: https://x.com/Brooooook_lyn/status/1881265313138041168 I was already considering using napi-rs for the native bindings, but if it support Wasm out of the box, then maybe that's the solution going forward? |
Yes, I believe this is the best solution. Wouldn't it be better if we had two packages, like limbo-web and limbo-node then? I think it would be even better if those were under the @tursodatabase org. Discoverability in NPM is a bit weird. |
@LtdJorge I am totally fine with different packages. Happy to move them under |
For what reason? It seems like there are enough tools to have it be in one package. I am not agains two packages I am just trying to understand the why! |
I should have said limbo-wasm and limbo-node. Limbo node would be the native package based on node-api (napi-rs). And limbo-wasm would be just the wasm builds for any wasm runtime. |
@penberg I'm working on the native Node bindings, should I put that in a different Napi-rs is giving me a bit of a hard time, although I have mostly everything implemented. I'll do bun and deno after node. Should the 3 be in the same bindings sub-crate? |
I changed the Next.js starter template on StackBlitz to following:
but there's something wrong wit the
limbo-wasm
package because it fails to load:The text was updated successfully, but these errors were encountered: