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

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

Closed
timfish opened this issue Jan 27, 2021 · 14 comments
Closed

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

timfish opened this issue Jan 27, 2021 · 14 comments
Assignees
Labels
bug Something isn't working

Comments

@timfish
Copy link
Sponsor

timfish commented Jan 27, 2021

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 Brooooooklyn self-assigned this Jan 28, 2021
@Brooooooklyn Brooooooklyn added the bug Something isn't working label Jan 28, 2021
@Brooooooklyn
Copy link
Sponsor Member

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: vercel/ncc#531

@Brooooooklyn
Copy link
Sponsor Member

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

@timfish
Copy link
Sponsor Author

timfish commented Jan 28, 2021

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
Copy link
Sponsor Author

timfish commented Jan 28, 2021

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
Copy link
Sponsor Author

timfish commented Jan 28, 2021

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
Copy link
Sponsor Member

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

@timfish
Copy link
Sponsor Author

timfish commented Jan 29, 2021

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

@timfish
Copy link
Sponsor Author

timfish commented Jan 29, 2021

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
Copy link

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
Copy link

guybedford commented Jul 26, 2021

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

  • The use of optionalDependencies means only the system-specific binaries are even available, so even if we could trace and emit the binaries we'd risk not including every platform.
  • Tracing what is being required is not possible because the require.resolve('${packageName}-${triple.platformArchABI}') statement is highly generic, and we don't do transitive function analysis to know the values of packageName in the above which would have helped with this tracing.

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
Copy link
Sponsor Author

timfish commented Jul 26, 2021

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
Copy link

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
Copy link

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
Copy link
Sponsor Member

I think this could be closed at present

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants