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

Switch to NAPI #138

Closed
kriszyp opened this issue Feb 3, 2022 · 15 comments
Closed

Switch to NAPI #138

kriszyp opened this issue Feb 3, 2022 · 15 comments

Comments

@kriszyp
Copy link
Owner

kriszyp commented Feb 3, 2022

lmdb-js currently uses the NAN API for the native add-on C/C++ code, but the NAPI API provides better ABI stability and would require fewer versioned builds. Supposedly Deno is working on supporting NAPI (https://twitter.com/biwanczuk/status/1487122575012548612), so this might be much easier way to support Deno + Node. However, this would require conversion of a lot of code, and not sure if everything we do is even supported by NAPI, so would need more investigation.

@littledivy
Copy link

littledivy commented Feb 4, 2022

👍 for this

I'm trying to get parcel to work under deno --compat + our [WIP] NAPI implementation. Currently, lmdb and msgpackr-extract are blocking us because of NAN.

I tried to port msgpackr-extract to NAPI here kriszyp/msgpackr-extract#7 but it seems to be broken (and I'm not too familiar with C++). Anyways, would love to help get lmdb-js ported to NAPI.

@kriszyp
Copy link
Owner Author

kriszyp commented Feb 4, 2022

Thank for the work on msgpackr-extract, I'll take a look at it! FWIW, you should already be able to run lmdb-js on Deno, as it supports the Deno FFI API and even includes some simple unit tests in the CI tasks. And msgpackr-extract is an optional dependency, so it is not necessary for lmdb-js or msgpackr to run on Deno. They should run fine without, msgpackr-extract just provides some performance boosting (although it would definitely be cool to get it working on Deno!).

But maybe you are already aware of all this and your goal is more of using parcel as a test piece for verifying the proposed NAPI implementation for Deno (I realized that your work has been mostly on Deno)? Certainly getting parcel running on Deno would be an impressive verification of binary add-on compatibility with all the different binaries that are used! Anyway, thanks for your efforts, the msgpackr branch will be a good chance for me to get more practice using NAPI as well, and hopefully pave the way for eventually similar work on lmdb-js.

@bartlomieju
Copy link

Hey @kriszyp!

But maybe you are already aware of all this and your goal is more of using parcel as a test piece for verifying the proposed NAPI implementation for Deno (I realized that your work has been mostly on Deno)?

Yeah, we tried to "polyfill" lmdb that parcel uses with the version available for Deno, unfortunately that doesn't work in this case - we need to make it available via require() calls, whereas Deno version requires us to import() is asynchronously.

Certainly getting parcel running on Deno would be an impressive verification of binary add-on compatibility with all the different binaries that are used!

This 100%. Parcel seems to be complex enough project that should allow us to validate that our NAPI integration is proper for broad usage.

@kriszyp
Copy link
Owner Author

kriszyp commented Feb 4, 2022

lmdb-js isn't working on Deno because it uses import instead of require, oh the irony 🤣 .

Since you are working on this in Deno, I have questions :). First, I am curious if I can just use NAPI as an "entry point" for the loading a module, and then cast the module exports to the V8 object (v8::Localv8::Object), and directly use the V8 apis in my code? Both lmdb-js and msgpackr-extract directly interact with the V8 APIs quite a bit, and it might actually be a lot easier to transition the code, at least in the short term/interim, to using NAPI as the entry point and then build classes and manipulate values with the V8 APIs. Of course I realize this undermines the goal of NAPI, ABI stability, but maybe possible as a transitional approach to getting it running (and then transition from internal use of V8 -> NAPI).

Also, I'm curious if the goal is NAPI would only be available in --compat mode? Or do you see it as eventually being more of the primary/first-class way to load binary add-ons (outside of WASM, at least)? Or should I not consider deno --compat to be second class?

Anyway, I'm impressed with how quickly you are making progress on this, I know NAPI in Deno is a big endeavor!

@bartlomieju
Copy link

lmdb-js isn't working on Deno because it uses import instead of require, oh the irony 🤣 .

It does work, it's just we can't use it for std/node polyfill ;)

Since you are working on this in Deno, I have questions :). First, I am curious if I can just use NAPI as an "entry point" for the loading a module, and then cast the module exports to the V8 object (v8::Localv8::Object), and directly use the V8 apis in my code? Both lmdb-js and msgpackr-extract directly interact with the V8 APIs quite a bit, and it might actually be a lot easier to transition the code, at least in the short term/interim, to using NAPI as the entry point and then build classes and manipulate values with the V8 APIs. Of course I realize this undermines the goal of NAPI, ABI stability, but maybe possible as a transitional approach to getting it running (and then transition from internal use of V8 -> NAPI).

I'm not sure I fully understand what you mean by "entrypoint" here. All in all, supporting NAPI is already a compromise to our goal of not exposing v8 APIs to users; so if/when NAPI lands in Deno I'm 99% sure we will not give direct access to v8 APIs that are not present in NAPI.

Also, I'm curious if the goal is NAPI would only be available in --compat mode? Or do you see it as eventually being more of the primary/first-class way to load binary add-ons (outside of WASM, at least)? Or should I not consider deno --compat to be second class?

Unclear at this point - but I would be leaning towards NAPI being always available. That would be third way to load binary addons (after WASM and FFI API). We are having internal discussion if deno --compat should be on by default in the next major version (ie. should Deno be a superset of Node APIs), but again, no decision yet.

@littledivy
Copy link

First, I am curious if I can just use NAPI as an "entry point" for the loading a module, and then cast the module exports to the V8 object (v8::Localv8::Object)

I don't think that's possible, NAPI by design hides the underlying layout and just exposes an opaque pointer for napi_value and friends. So casting to something will really be dependent on how Node and Deno represent these values in their implementation.

Certainly getting parcel running on Deno would be an impressive verification of binary add-on compatibility with all the different binaries that are used!

Yessss!

kriszyp added a commit that referenced this issue Feb 9, 2022
@kriszyp
Copy link
Owner Author

kriszyp commented Mar 7, 2022

Ok denosaurs, @littledivy and @bartlomieju, I've published a pre-release version of v2.3.0 (https://www.npmjs.com/package/lmdb/v/2.3.0-alpha.1) with the rewrite to use NAPI. I have verified running parcel with this, so this hopefully this should give you something viable to test Deno with NAPI. A few notes on this:

  • This is prelease, there are still some a number of bugs and optimizations to be done to finish this release.
  • This actually does still use some V8 APIs that are optionally loaded for performance speedup; these are loaded dynamically if running on a compatible version of Node. I believe this works properly on *nix systems, but not on Windows, so the Windows prebuild is NAPI only.
  • @bartlomieju , I think you mentioned that require() calls weren't working; I assume that was for trying to run FFI. In the NPM package, the CJS files are in dist folder, and referenced by exports['.'].node.require, which I presume should get used in node compatibility mode. If there is another way to do that for better compatibility, let me know.
  • If you are having problems, and you have a build of Deno with NAPI available, I could take a look.

Anyway, hopefully that gives you something to work with for the parcel-on-deno project, good luck! And long-term this will certainly be a great way for high-performance binaries to run on Deno, so thank you for your efforts!

@bartlomieju
Copy link

@bartlomieju , I think you mentioned that require() calls weren't working; I assume that was for trying to run FFI. In the NPM package, the CJS files are in dist folder, and referenced by exports['.'].node.require, which I presume should get used in node compatibility mode. If there is another way to do that for better compatibility, let me know.

Sounds good, require() doesn't work yet, because NAPI hasn't been yet wired to it, but once we land support for NAPI we will make it work in require() calls.

Thanks for keeping us updated!

@kriszyp
Copy link
Owner Author

kriszyp commented Mar 7, 2022

Looking into the PR a little, would it be reasonable/preferable for me to specifically target Deno and use Deno.core.napiOpen(lib); (from https://github.com/denoland/deno/pull/13633/files#diff-d60499df4f6eb52a7ae03fc41cce30ebdc258196583ce8682c9a3396d3f54d52R18) to load the binary (rather than relying on node compatible require)? I suppose this might still be up in the air, but will Deno.core.napiOpen possibly be available outside of node compatibility mode?

@bartlomieju
Copy link

I suppose this might still be up in the air, but will Deno.core.napiOpen possibly be available outside of node compatibility mode?

Currently that's not very probable. As a rule of thumb Deno.core.* APIs are considered private and can change at any moment, so it's better no to rely on them. So I would suggest to keep using require() only

@kriszyp
Copy link
Owner Author

kriszyp commented Mar 18, 2022

@bartlomieju I'm also curious about the intended or suggested means of distribution. Presumably lmdb-js could run in Deno as a Node-style transient dependency package within a set of NPM installed packages alongside Parcel and friends, and loaded via node-compatible require calls. However, it seems like it would be nice if it could still be available to directly be used from Deno's module registry at https://deno.land/x/lmdb as well, so users could import { open } from 'https://deno.land/x/lmdb/mod.ts' (still necessitating --compat mode, I'd assume). It seems maybe a few options for that:

  • Continue some of the wild hacks used here https://deno.land/x/[email protected]/mod.ts#L14, to manually find and download the right binary (for the platform), write it OS temp location, and load it (with a file path based require instead of Deno.dlopen)
  • Require it to be installed with a package manager first, and have mod.ts just load that:
try {
  export const { ... } = require('lmdb');
} catch (error) {
  throw new Error('Please install lmdb-js as a package first (npm install lmdb)');
}
  • Some other Deno approved/suggested means of distribution?

@bartlomieju
Copy link

Hi @kriszyp.

I'm also curious about the intended or suggested means of distribution. Presumably lmdb-js could run in Deno as a Node-style transient dependency package within a set of NPM installed packages alongside Parcel and friends, and loaded via node-compatible require calls.

Right, I think this will be the main way of consuming at it will work transparently for users.

However, it seems like it would be nice if it could still be available to directly be used from Deno's module registry at https://deno.land/x/lmdb as well, so users could import { open } from 'https://deno.land/x/lmdb/mod.ts' (still necessitating --compat mode, I'd assume).

There's no decision yet if NAPI will require --compat mode, but other than that this takes sense - the open function should sniff the current OS and load appropriate compiled lib file. It is unfortunate that currently you need to manually fetch and store the file somewhere, WASM files suffer from the same fate. There is a proposal that would solve this elegantly (https://github.com/lucacasonato/proposal-evaluator-attributes), but it's only at Stage 1. I could imagine we might special case NAPI in such a way that Deno would be able to fetch (and cache) remote files for the purpose of NAPI before that proposal moves forward, but again no decision has been made yet.

I'm curious to learn your thoughts on this matter, which option in your mind would be the best from the DX standpoint?

@kriszyp
Copy link
Owner Author

kriszyp commented Mar 22, 2022

Yes, this proposal certainly does seem like it could provide a more elegant approach to loading binary modules. As far as my dev experience, simply installing packages with npm (or yarn or whatever) is the easiest to support, since that is already the supported approach in node. I just thought Deno was continuing to push for the URL-based module system, so wanted to support idiomatic, best-practice Deno packaging of modules as much as possible. But I certainly don't mind waiting to see if and when there is a better supported method that comes along. Using npm (and --compat) seems fine for now.

@kriszyp
Copy link
Owner Author

kriszyp commented Mar 22, 2022

Anyway, thanks for the help. BTW, do you have any estimate on when you are hoping NAPI might land?

@bartlomieju
Copy link

Anyway, thanks for the help. BTW, do you have any estimate on when you are hoping NAPI might land?

Hoping to get it in in 1.21 or 1.22, so in April or May.

@kriszyp kriszyp closed this as completed Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants