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

Feature Request: Allow passing in the Require / or actual loaded "better_sqlite.node" #972

Open
NathanaelA opened this issue Mar 21, 2023 · 15 comments

Comments

@NathanaelA
Copy link

This is similar to #866 & #126
When webpacking (or in my case Rollup) and then the "bindings.js" attempts to look in the completely wrong location. I have found a hack that I think might be a better feature in the better-sqlite than me manually adding the code to the better-sqlite.

So here is the issue, I have the better_sqlite3.node copied to an extracted location in my electron application, however rollup (& webpack) replaces the "require" with their own require making it impossible for me to use the nativeBinding to point to it because a require just uses the replaced call. However, if replace

} else {
addon = require(path.resolve(nativeBindingPath).replace(/(\.node)?$/, '.node'));
}

the call inside the lib/database file with:

    const create = nodeModule.createRequire || nodeModule.createRequireFromPath;
    const requireMe = create(  require("node:url").pathToFileURL(__filename).href  );
     addon = requireMe(path.resolve(nativeBindingPath).replace(/(\.node)?$/, '.node'));

Then I can create my own "require" call that actually calls the normal node/electron require statement. (Webpack has I believe a __non_webpack_require__ (I think it is called this) function which is the way you can access it in webpack). And you can also sometimes get access to it via global.require, etc...

Now the feature idea or request is to allow me to pass in the require function call you will use for the nativeBinding OR to allow me to manually load the binding myself and pass it in, so that you can just use it.

@JoshuaWise
Copy link
Member

options.nativeBinding already exists.

@NathanaelA
Copy link
Author

NathanaelA commented Mar 21, 2023

@JoshuaWise - Sorry, you misunderstood me. options.nativeBinding is just a path to where the better_sqlite.node file is at. This doesn't help me in this specific set of corner cases. ☹️

The issue I'm running into is:

The require on line 50 of your database.js is actually replaced by webpack/rollup with their own custom require function, so it no longer loads .node files. So even passing in the nativeBinding where the .node file is, is useless in this case. Most the time webpack/rollup are fairly good about not replacing require in cases of loading .node, but the detection seems to be failing on your code base. I can hack around for a few more hours on the rollup config to try and make it work, or I work around it using the node:module class (my example above) to get back a real require command (since webpack & rollup don't mess with this) and I'm done.

So I am proposing a feature added to better-sqlite to either allow me to pass in the actual require command that addon = require(path.resolve(nativeBindingPath).replace(/(\.node)?$/, '.node')); calls. Or I think even better yet, make the code base even simpler and remove the options.nativeBindings) and I can just pass in the addon (maybe called nativeNodeModule?) and I am responsible for loading it, so that I can do whatever "hacks" are needed in my code base rather than inside better-sqlite.

I'm willing to submit a PR for either feature if you are on board. I honestly think the second way (me being responsible to load and pass your code the already loaded better-sqlite.node module), makes the most sense because then the developer can workaround any corner case in any webpacker in the future too in their own code base....

I am aware I can exclude better-sqlite (& bindings & file-uri-to-path) node_modules to make sure I don't get the requires replaced. However, this is exponentially more costly in cpu/disk time. Each disk hit for each of the 14 separate files in better-sqlite, +2 for bindings, +2 for file-uri-to-path the is tremendously more expensive then having it all of those in a single minimized vendor.js packed file.

@JoshuaWise JoshuaWise reopened this Mar 21, 2023
@NathanaelA
Copy link
Author

@JoshuaWise would you prefer a PR for passing in the require statement or the actual already loaded better-sqlite.node file?

I assume if it is require that options.require is fine as the parameter, and if it is the .node file, perhaps options.nativeNodeModule or do you have a better optional parameter?

@mceachen
Copy link
Member

mceachen commented Apr 25, 2023

Fixed by #974, thanks @destyk!

@NathanaelA
Copy link
Author

@mceachen - This DOES NOT fix my issue. PLEASE RE-OPEN.

I'm actually using Vite/Rollup and it replaces the "require" also, but in a different way. The proposal I'm proposing is a lot more generic and not dependent upon which bundler people are using, and totally future safe...

Either allow me to pass in the node "require" function, or I the dev can externally require the .node file and pass it in for usage.. ;-) (I prefer the second method, over the first, but either would solve the issue).

Again, I'm willing to make a PR if you can tell me which way you prefer!

@mceachen mceachen reopened this Apr 26, 2023
@JoshuaWise
Copy link
Member

JoshuaWise commented May 14, 2023

@NathanaelA After giving this some thought, I think options.nativeBinding could accept a loaded addon object, and it should keep the current behavior if a string is passed to it. If you open a PR we can try to get it merged soon.

I apologize for taking so long to address this issue. Work and life have been quite busy.

@destyk
Copy link
Contributor

destyk commented Jun 19, 2023

@NathanaelA After giving this some thought, I think options.nativeBinding could accept a loaded addon object, and it should keep the current behavior if a string is passed to it. If you open a PR we can try to get it merged soon.

I apologize for taking so long to address this issue. Work and life have been quite busy.

resolved: #974

@SamTV12345
Copy link

@NathanaelA I am in the same situation. I need to rollup a library but when I do that and need the sqlite database type it throws the before mentioned error. Is there a workaround or do we have to wait until #974 is merged?

@SamTV12345
Copy link

@destyk If you have some time could you please explain if @NathanaelA and my problem are solved with your pull request? If the maintainers are busy I'll publish your pull request to npmjs so I can use it in etherpad in order to move forward with my rollup configuration.

@NathanaelA
Copy link
Author

@SamTV12345 - #974 should fix the issue because you can then pre-load the native module from where you want to and just pass it in as nativeBinding. My solution was similar, but also required a forked version that I didn't bother releasing anywhere just because it was a simple one off, using the technique below. @destyk actually created the PR to fix this before I could. 👍 to them...

As a minor aside, you do not need to create a whole new npm package & do a npm publish, you can simple do a npm pack of the code and then use the fixed plugin .tgz file only locally. For one off's like this I don't tend to clutter up npm and create the extra npm packages when all I need is a fixed/modified version for a short amount of time (although this has been a bit longer than I expected). You can then copy that file into your project (I tend to use a subfolder) do a npm i ./subfolder/better-sqlite3.tgz

@SamTV12345
Copy link

@SamTV12345 - #974 should fix the issue because you can then pre-load the native module from where you want to and just pass it in as nativeBinding. My solution was similar, but also required a forked version that I didn't bother releasing anywhere just because it was a simple one off, using the technique below. @destyk actually created the PR to fix this before I could. 👍 to them...

As a minor aside, you do not need to create a whole new npm package & do a npm publish, you can simple do a npm pack of the code and then use the fixed plugin .tgz file only locally. For one off's like this I don't tend to clutter up npm and create the extra npm packages when all I need is a fixed/modified version for a short amount of time (although this has been a bit longer than I expected). You can then copy that file into your project (I tend to use a subfolder) do a npm i ./subfolder/better-sqlite3.tgz

So nativebinding points to the path where the native binding is. Where do I get the native bindings? Sorry if this is a noob question I haven't really worked a lot with SQLite in JavaScript. And how could I do that platform independant?

Yes. But once I need to do it in a CLI I can't use this method that is why.

@NathanaelA
Copy link
Author

Sorry if this is a noob question

Everyone starts somewhere, so I never consider any question a noob question. I'll try to explain the entire process of what happens.

Where do I get the native bindings?

Certain Node NPM modules have c/c++ code that gets compiled into a native module (i.e. native bindings) for that specific OS platform. These native module/bindings .node files are located in different places depending on the package. Better-SQLite3's .node file it is downloaded or built into the 'node_modules/better-sqlite3/build/Release/better_sqlite3.node'. This is the native code that has the sqlite library built into it so that your app can actually talk to sqlite from node. The .node files are platform specific, and node version specific. So you can NOT run the windows built version on Linux, or vise-versa. Nor can you run a node 14 version of the file on a node 20 instance. So when you first install better-sqlite3, it looks at your OS platform and your node version and downloads a pre-compiled version of this library (if it exists), if it doesn't exist then it builds it using your c/c++ compiler you have on your machine.

Now normally that part is fairly seamless, the issue that I reported comes with a couple different corner cases:

  1. Using a different bundler, that replaces the "require" with its own.
  2. Using a different bundler that bundles all the code (and you aren't excluding certain pieces from being bundled up)
  3. Using Electron and bundling an app.

In my case I actually have all three issues, I am using something other than webpack (rollup) and it does replace the require. I also do NOT want to leave the better-sqlite javascript package outside of the bundles I'm creating and finally when you bundle an electron app the paths change and will confuse the native bindings loader when you bundle in the sqlite javascript.

So when I ran into this issue because when I packaged it in electron. (1) rollup replaced the require, (3)and the .node file is no longer where anything expects it. So my first solution was to actually get node to give me the the original "require" function which you can see the code in the original issue, and then use the original "require" to load the native binding from where it now is located. Right now I have to hack in some code inside my custom version of the better-sqlite3 library to allow me to pass in my custom requires and path to the .node file. Using #974 I will do basically the same thing, other than I will do all the code for the native "require" statement, and then manually load the binding, and then pass in the node binding to better-sqlite all from my database class. This will then keep all the custom code outside of better-sqlite3, rather than me having to have a custom version of better-sqlite3.

And how could I do that platform independant?

You can't - .node files are not only Node (or electron) version specific, but also platform specific. In my case I build an electron app that I currently distribute for Windows and Linux. I do actually build all versions of my app and sign them on my Linux machine. Each time I upgrade Electron I do the following.

  1. I prebuild all my windows native bindings on a windows machine (I have a couple different native bindings), and copy them to my "buildResources" folder on my linux machine.
  2. I also pre-build the linux version using a custom docker container (because of this Crash when using prebuilt v8.5.2 with Electron 26.1.0 #1044 (comment)) to build better-sqlite3 on my linux machine and I also copy it to my buildResource folder.
  3. My electron building script I have it setup to pull the "proper" version of the node binding file depending on which platform I am building and name it the same thing in the proper output directory. This allows me to build any platform from my linux machine.

Yes. But once I need to do it in a CLI I can't use this method that is why.

The build of the native bindings .node file are manual steps (for me I do have to kick them off and copy the files). They could be automated, but I only have to do it every couple months, so I don't bother adding all the work to automate it.

These files after being build can be put into your repo, and then updates to them can also be pushed to your repo, keeping them in sync with the version of node/electron you are using for the project. In addition your custom version of the better-sqlite3 (until they finallly merge #974 ) can also be npm packed into a .tgz and this can also be put into your repo. I have used this technique for many years to have custom one-offs with my code so that a fresh checkout has everything and everything is already linked up for me (or anyone else) to just do a npm i and npm run build to release the project.

Hopefully that helps, if you need more info please feel free to email me. ;-)

@SamTV12345
Copy link

Thank you so much for taking the time to write such a long and detailed explanation. It is sad that there is not pure JavaScript implementation of SQLite but I guess this is not trivial with all the years of development and of course the speed that would degrade. I can now understand why there was a comment in the project I maintain with the content that SQLite should only be manually on an instance as a slight divergence in versions could easily crash that instance. The big question is how I'll implement that manual step elegant as now all the code is injected into the one javascript file. Maybe I'll put the SQLite code into a separate folder and require that from there. Anyways thanks again for the help.

@NathanaelA
Copy link
Author

NathanaelA commented Sep 3, 2023

The easiest way I found to do that step is to have one of my build steps copy the file from my buildResources folder to the final destination before packaging everything up -- my buildResources folder tends to keep anything that is a native binary module per platform, and all other static things like images. If you are creating an Electron app, you can use Electron builder's JSON/YAML configuration file to copy those resources prior to it turning it into a Deb (Linux) or Exe (Windows) file. If you are just using Rollup (like for a Tauri app), I'm pretty sure their is a copy rule you can add to allow you to copy things while rollup does its building of the bundled JS file.

As for sqlite crashing, the only issue which I have personally run into is the linux glibc version issue, but that is why I use a custom docker (exactly like I use windows) to build all of my .node files that I then drop into my build resources folder, this way the .node files that are actually in my node_modules (and linked to glib 2.35) are never used for the final build, they are just for while I'm working on code and testing things...

@bobogogo1990
Copy link

bobogogo1990 commented Dec 20, 2023

pass better-sqlite3.node has supportted. And

import { createRequire } from "module";
const nodeRequire = createRequire(import.meta.url);

use nodeRequire to require the better-sqlite3.node works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

6 participants