kriszyp / lmdb-js

Simple, efficient, ultra-fast, scalable data store wrapper for LMDB
Other
505 stars 41 forks source link

Bun support #208

Closed rizrmd closed 1 year ago

rizrmd commented 1 year ago

Just tried installing lmdb with bun and running sample code:

import { open } from 'lmdb'; // or require
let myDB = open({
    path: 'my-db',
    // any options go here, we can turn on compression like this:
    compression: true,
});
await myDB.put('greeting', { someText: 'Hello, World!' });
myDB.get('greeting').someText // 'Hello, World!'
// or
myDB.transaction(() => {
    myDB.put('greeting', { someText: 'Hello, World!' });
    myDB.get('greeting').someText // 'Hello, World!'
});

And this happen:


error: Could not resolve: "v8". Maybe you need to "bun install"?

import { setFlagsFromString } from 'v8';
                                   ^
/Users/r/Desktop/projects/a/node_modules/lmdb/node-index.js:5:36 

Bun does not use v8 like deno/node, it uses JavascriptCore from Apple. I know lmdb officially only supports node and deno. Can we add one for bun?

kriszyp commented 1 year ago

Yes, I am definitely aware that bun does not use v8, and that is exactly why lmdb-js is careful to only load V8 code if the runtime says it is V8: https://github.com/kriszyp/lmdb-js/blob/master/node-index.js#L12 However, Bun seems to have become confused lately, in earlier versions it did not report v8, but now Bun has a regression and process.versions on bun reports:

{
  node: "18.13.0",
....
  v8: "10.8.168.20-node.8",
  modules: "108"
}

(note, that "modules" is the specifically the NAN version that interfaces with V8, which Bun also does not support).

This should be filed as a bug with Bun, as it is clearly lying and causing it owns failures in the process.

rizrmd commented 1 year ago

Just posted this issue on bun. I hope it get attention. Anyway, I think bun need to assume v8 to shim some npm modules to make it work. Also there is process.isBun to reliably assume that runtime is bun.

Here is my output for console.log(process):

image
Jarred-Sumner commented 1 year ago

I think we could remove process.versions.modules, but I'm not sure about process.versions.v8. A decent amount of code expects process.versions.v8 to be a string.

https://sourcegraph.com/search?q=context:global+process.versions.v8.+lang:js&patternType=standard&sm=1&groupBy=repo

https://sourcegraph.com/search?q=context:global+process.versions.modules+lang:js&patternType=standard&sm=1&groupBy=repo

kriszyp commented 1 year ago

@rizrmd v2.7.5 should have support for this latest breaking changes in Bun. Note, that this should enable the standard get/put functionality, but doesn't enable asynchronous transactions. This requires event passing between threads, via the napi_call_threadsafe_function which currently still segfaults in bun, and is reported at https://github.com/oven-sh/bun/issues/158#issuecomment-1126624085 (to be fair, I think deno segfaults with that function as well, guess it is hard to implement). I have done a fair bit of work over the last year to try get lmdb-js compatibility in Bun, but asynchronous transactions just won't work without that piece. Anyway, hopefully v2.7.5 will get you going with LMDB on Bun.

@Jarred-Sumner I understand the desire to get this working. I do think we would be better off pushing the ecosystem to adopt better practices (push those that are misusing process.versions.v8 to change rather than those that are correctly using it as detection for V8) instead of just a pursuit of adoption/compatibility with node. This is kinda the same reason we have ridiculous user agent strings in browsers, because browser vendors have placed compatibility/adoption over pushing better practices. And in the examples above, it actually looks like most of the examples in the referenced search really are doing V8 specific actions (that wouldn't make sense in JSC). That being said, this is trivial fix (just using !process.isBun), so certainly not difficult to deal with.

rizrmd commented 1 year ago

Ah, thank you. So I guess this means bun is supported right ?

I will close this issue, and if someone needs async transaction, they should open new issue.

kriszyp commented 1 year ago

I guess this means bun is supported right ?

I have definitely been trying to maintain support for Bun since it released NAPI support.