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

Fix for using bindings.js with eval #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pverscha
Copy link

Hi!

I found a lot of issues and bug reports that all seem to originate from "bindings.js" in combination with the eval function. In this specific case, the exports.getFileName(); returns undefined, causing exports.getRoot() to crash with the error message Cannot read property 'indexOf' of undefined at Function.getFileName (bindings.js:180) (see: #54).

This PR fixes this issue by checking if a valid filename was found in exports.getRoot() and returns undefined there if not. This, however, means that the module_root is set to the root-folder of your application, not the node_modules/{package} folder of the package that's trying to find it's node-bindings. Since we've got no other information about the package root we're looking for, other than the ".node"-filename, I've added a heuristic that looks for the package name (in the node_modules-folder) that best matches the ".node"-filename.

This approach seems to work very well, especially for "better-sqlite3" that suffers from this issue when used in combination with electron and web workers.

Would you be so kind to review and accept my PR?

@pverscha
Copy link
Author

@TooTallNate Could you take a look at this?

@pratyakshs
Copy link

@pverscha this patch still throws the Could not locate the bindings file. error when used with @airbnb/node-memwatch + webpack. Let me know if you need help reproducing.

@aarowman
Copy link

Has there been any progress on this?

@relines
Copy link

relines commented Jan 9, 2024

Hi!

I found a lot of issues and bug reports that all seem to originate from "bindings.js" in combination with the eval function. In this specific case, the exports.getFileName(); returns undefined, causing exports.getRoot() to crash with the error message Cannot read property 'indexOf' of undefined at Function.getFileName (bindings.js:180) (see: #54).

This PR fixes this issue by checking if a valid filename was found in exports.getRoot() and returns undefined there if not. This, however, means that the module_root is set to the root-folder of your application, not the node_modules/{package} folder of the package that's trying to find it's node-bindings. Since we've got no other information about the package root we're looking for, other than the ".node"-filename, I've added a heuristic that looks for the package name (in the node_modules-folder) that best matches the ".node"-filename.

This approach seems to work very well, especially for "better-sqlite3" that suffers from this issue when used in combination with electron and web workers.

Would you be so kind to review and accept my PR?

I reported an error after using your bindings.js:

could not find module root given file: “”.Do you have a ‘package.json’ file? at t.getRoot

@mepanko91
Copy link

I had same issue. I solved it by using better-sqlite3 module. It has a nativeBinding option. This option can be used to specify file path of sqlite native .node dependency file. If this option is provided then better-sqlite3 module won't use bindings module to locate .node file so the error from bindings module doesn't occur

let dbOptions = { fileMustExist: true };
if (appEnv.inPKG) {
  dbOptions.nativeBinding = path_join(
    appEnv.pkgRoot,
    "node_modules/better-sqlite3/build/Release/better_sqlite3.node"
  );
}
let db = new Database(dbPath, dbOptions);
// now works..

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

Successfully merging this pull request may close these issues.

5 participants