TooTallNate / node-bindings

Helper module for loading your native module's `.node` file
MIT License
412 stars 95 forks source link

Fix for using bindings.js with eval #66

Open pverscha opened 4 years ago

pverscha commented 4 years ago

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: https://github.com/TooTallNate/node-bindings/issues/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 commented 4 years ago

@TooTallNate Could you take a look at this?

pratyakshs commented 4 years ago

@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 commented 1 year ago

Has there been any progress on this?

relines commented 10 months ago

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 commented 6 months ago

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..