napi-rs / node-rs

Node.js bindings ❤️ Rust crates
https://node-rs.dev
MIT License
1.14k stars 33 forks source link

@node-rs/helper incompatible with asset relocation #316

Closed timfish closed 2 years ago

timfish commented 3 years ago

Both ncc and electron-forge rely on @vercel/webpack-asset-relocator-loader to locate and copy native node assets.

@vercel/webpack-asset-relocator-loader supports many conventions, works with wildcards, __dirname and has special cases for historic ways to load a native binary (ie. require('bindings')(...), nbind.init(..), etc). However, it's analysis fails to make sense of the binding loading code in @node-rs/helper because it's too complex.

To reproduce:

package.json

{
  "name": "ncc-test",
  "version": "1.0.0",
  "main": "dist/index.js",
  "license": "MIT",
  "scripts": {
    "start": "node ./dist/index.js -v",
    "build": "ncc build index.js -o dist"
  },
  "devDependencies": {
    "@vercel/ncc": "^0.27.0"
  },
  "dependencies": {
    "@node-rs/crc32": "^1.0.0"
  }
}

index.js

const crc = require("@node-rs/crc32");
const out = crc.crc32(Buffer.from([1, 2, 3, 4, 5, 6, 7, 8, 9]));
console.log("out", out);

Run

yarn && yarn build && yarn run

Result: Can not find node binding files from @node-rs/crc32-darwin-x64 and /Users/timfish/Documents/Repositories/ncc-test/dist/crc32.darwin-x64.node

Brooooooklyn commented 3 years ago

I think the problem with ncc is not caused by __dirname, since __dirname seems to work fine. I think this problem is caused by require.resolve which is rewritten by ncc into __nccwpck_require__(44).resolve and the 44 chunk is the empty chunk.

Seems like the other libraries meets the similar problem here: https://github.com/vercel/ncc/issues/531

Brooooooklyn commented 3 years ago

I have tried with ncc build -e '@node-rs/helper' index.js -o dist, and works fine.

timfish commented 3 years ago

Marking @node-rs/helper as external means that it and anything it loads will not be bundled, so as a minimum you'd need to ship node_nodules/@node-rs/*.

This works because ncc supports statically analysing os.

const os = require("os");
const crc = require(require.resolve(
  `@node-rs/crc32-${os.platform}-${os.arch}`
));
const out = crc.crc32(Buffer.from([1, 2, 3, 4, 5, 6, 7, 8, 9]), 0);
console.log("out", out);

and results in this:

ncc: Version 0.27.0
ncc: Compiling file index.js
  3kB  dist/index.js
681kB  dist/crc32.darwin-x64.node   <- native binary gets included in bundle output
684kB  [604ms] - ncc 0.27.0
timfish commented 3 years ago

This works:

try {
  // trick bundler into locating and including all required .node binary assets
  const num = Math.random();
  require(require.resolve(`@node-rs/crc32-${num}.node`));
} catch (_) {}

const crc = require("@node-rs/crc32");
const out = crc.crc32(Buffer.from([1, 2, 3, 4, 5, 6, 7, 8, 9]));
console.log("out", out);
timfish commented 3 years ago

If modify @node-rs/crc32/index.js to this, it bundles and runs correctly

const { loadBinding } = require('@node-rs/helper')

// This will never be called at runtime but bundlers will copy any .node assets
if (process.uptime < 0) {
  require(require.resolve(`@node-rs/crc32-${Math.random()}.node`));
}

const binding = loadBinding(__dirname, 'crc32', '@node-rs/crc32')

module.exports = {
  crc32: function crc32(input, crc = 0) {
    const _input = Buffer.isBuffer(input) ? input : Buffer.from(input)
    return binding.crc32(_input, crc)
  },
  crc32c: function crc32c(input, crc = 0) {
    const _input = Buffer.isBuffer(input) ? input : Buffer.from(input)
    return binding.crc32c(_input, crc)
  },
}
Brooooooklyn commented 3 years ago

Could it be fixed in @node-rs/helper ? Hack it in every libraries useing the napi-rs is too ugly...

timfish commented 3 years ago

It doesn't work from @node-rs/helper because it can't statically analyse packageName.

timfish commented 3 years ago

This is the simplest I've managed to get it:

if (!process) {
  require(require.resolve(`@node-rs/crc32-${process}.node`));
}

Another option would be to drop the use of @node-rs/helper, @napi-rs/triples and generate a loadBindings() with static module names via the napi cli?

MorganLindqvist commented 3 years ago

I would like this to be fixed without having to exclude node-rs from webpack. The same bug is discussed here. Currently we are forced to use other bcrypt modules since there is a requirement on that webpack shall be used. Since the other bcrypt modules are slower I would like to move back to your implementattion as soon as it is possible to use with webpack.

guybedford commented 3 years ago

Unfortunately there are a number of things here that make static emission for ncc very difficult here:

If the above weren't the case, a static emission of all binaries could be possible to work, and there could be a middle ground here. But it could be more optimal to continue to rely on the optionalDependencies pattern being applied and simply treat @node-rs/* as external dependencies in ncc builds.

timfish commented 3 years ago

The thorn in the side of anyone publishing and maintaining a native node module is Electron.

Most Electron devs seem to be using webpack to keep node_modules out of their distributed apps and keep them a reasonable size. I recently added this lovely hack to electron-forge so that @vercel/webpack-asset-relocator-loader can be used in the Electron renderer rather than the ancient fork it was previously using.

For Electron, it's quite common to bundle for a different arch to your current nodejs. Packaging for ia32 on Windows is common so a single app can target both x64 and ia32. Packaging for arm64 on macOS x64 is common since nobody is offering arm64 Mac CI.

With optionalDependencies how would the Electron tooling install the right binaries? The shortest path to joy seems to be to load from a fixed location and copy or download prebuilt binaries through an install script. Platform and arch can then be overridden through the standard environment variables. The outstanding issue is that the current tooling (electron-rebuild) doesn't actually call install scripts and only special cases gyp 😭.

BasixKOR commented 2 years ago

This is quite problematic for Webpack usage as well. The workaround mentioned above doesn't work for Webpack since Webpack converts require calls to a dummy responds with MODULE_NOT_FOUND.

// ...snip...
                return __webpack_require__("../node_modules/@node-rs/helper/lib sync recursive")(__webpack_require__("../node_modules/@node-rs/helper/lib sync recursive").resolve(`${packageName}-${triple.platformArchABI}`, { paths: [dirname] }));
// ...snip...

/***/ "../node_modules/@node-rs/helper/lib sync recursive":
/*!*************************************************!*\
  !*** ../node_modules/@node-rs/helper/lib/ sync ***!
  \*************************************************/
/***/ ((module) => {

function webpackEmptyContext(req) {
    var e = new Error("Cannot find module '" + req + "'");
    e.code = 'MODULE_NOT_FOUND';
    throw e;
}
webpackEmptyContext.keys = () => ([]);
webpackEmptyContext.resolve = webpackEmptyContext;
webpackEmptyContext.id = "../node_modules/@node-rs/helper/lib sync recursive";
module.exports = webpackEmptyContext;

/***/ }),
BasixKOR commented 2 years ago

Just created a Webpack loader to resolve this issue: https://github.com/BasixKOR/node-rs-loader

I think ncc could use similar solutions, although I'm unsure my solution works generally.

Brooooooklyn commented 2 years ago

I think this could be closed at present