prebuild / node-gyp-build

Build tool and bindings loader for node-gyp that supports prebuilds
MIT License
109 stars 38 forks source link

The loading algorithm doesn't work for Webpack-bundled applications when dependencies are bundled into them. #22

Open millimoose opened 5 years ago

millimoose commented 5 years ago

When using Webpack to bundle a Node application into a single .js file, node-gyp-build cannot unambiguously load the correct .node file.

In a bundled application, Webpack can provide as __dirname either "/" which is the default, the input file's path relative to the build context, or Node's regular behaviour

To support bindings based native modules, you can copy the built .node files into build/Release relative to the bundle file, because bindings is passed the name of the .node file. When node-gyp-build tries to load build files from that folder, if it contains multiple .node files from multiple modules, it will grab the first one in it for every module that uses node-gyp-build.

E.g. if my project uses argon2 and leveldown and builds them from source, I will have the bundled output structure:

foo
|-- index.js
|-- build
    |-- Release
        |-- argon2.node
        |-- leveldown.node

but either library trying to load its bindings file will just end up loading argon2.node.


For similar reasons prebuilds are completely impossible to use, because all flavours of prebuild for a given runtime and ABI will have the same filename (such as node-napi.node), and since there's only one bundle file its containing directory won't disambiguate between them.

millimoose commented 5 years ago

My easiest available workaround is just not bundling in libraries, but that adds in my case 500MB of production dependencies - of which I only need 12MB - and makes my build a whole lot slower. Either that or I'm stuck figuring out which exact libraries I need to keep unbundled by hand and package them separately. (Which is potentially laborious, since Webpack cannot load bundled libraries from unbundled ones, so I have to include the entire dependency tree.)

vweevers commented 5 years ago
|-- build
   |-- Release
       |-- argon2.node
       |-- leveldown.node

A flat structure like that is ill-advised in any case, because some native modules have multiple .node files, or other additional files (e.g. statically linked libs).

millimoose commented 5 years ago

That may be the case, but assuming I want to bundle dependencies, I don't have the option of having the files loading the bindings be in different containing folders, and node-gyp-build won't look for them elsewhere. (This might change with Webpack 5 which will support vendor chunks for Node targets, but that's not currently the case.) I can probably set up the structure any way I want were a workable one supported.

I will grant that this might be an issue that's not completely solvable on the side of node-gyp-build, since bundling Node applications isn't all that common, and even less common is bundling their dependencies as opposed to leaving them as externals - so there aren't any established practices being violated. But adding a non-opinionated mechanism to locate bindings disambiguating using something else than just the __dirname might be a step forward.

Even something like an additional optional parameter that explicitly specifies an additional path relative to __dirname to search would probably help; e.g. leveldown would call:

require('node-gyp-build')(__dirname, 'leveldown')

and node-gyp-build would search both: __dirname/build/Release/*.node and __dirname/leveldown/build/Release/*.node. (Bonus: this would allow for a more informative error message that says "tried to search for bindings for leveldown in the following locations and failed: ..." instead of giving the same error message for every library with a stack trace that hasn't been source-mapped yet.)

("Just don't bundle dependencies" is my current path forward but it's not trivial to undo it on an established codebase - not all my libraries behave well as externals for reasons I have yet to diagnose. And it's impossible to just have native modules as external, because webpack doesn't automatically handle their dependencies, so I'd have to specify all of those myself - since webpack can't load bundled modules from external ones.)

vweevers commented 5 years ago

Could this be solved with a webpack plugin, perhaps? That rewrites require('node-gyp-build')(..) calls.

millimoose commented 5 years ago

It might be possible, and it would be orthogonal to node-gyp-build and libraries that use it, but it seems deceptively complex. E.g. require probably works for most cases now, but it's already possible to ship a multi-format library now which is written using ES6 modules and distributed in that format as well as transpiled to CommonJS; and Node 12 plans to enable ES6 module support when it goes LTS in October. Doing source-to-source transformations correctly is probably a job for Babel actually to make sure sourcemaps don't break just to add one more egg to juggle.

Now that I'm rubberducking this, what could work while remaining relatively simple is adding a shim module like node-gyp-build-webpack that would take the input __dirname webpack can emit that contains the original module name, alias 'node-gyp-build' to it instead, and pass a path to where I'm actually placing the binaries.

aminya commented 4 years ago

This is not specific to Webpack. Could you change the title? It happens with any bundler which copies the files to some other folder.

https://github.com/atom-ide-community/fuzzaldrin-plus-fast/pull/13

aminya commented 4 years ago

@vweevers Close this as per https://github.com/prebuild/node-gyp-build/pull/35#issuecomment-716136520.

Add your comment to the Readme.

vweevers commented 4 years ago

@aminya This issue is slightly different from the one you had, so keeping it open.

aminya commented 4 years ago

That's the exact issue. All the bundlers change the path of the files. That results in "__dirname" being incorrect.

vweevers commented 4 years ago

@aminya The two issues are both related to __dirname, but not the same. Your issue was about using a bundler in your native addon, which changed the location of the package entry point and thus broke __dirname expectations.

This issue on the other hand, is about consuming a native addon with a bundler, especially multiple native addons, and especially with bundlers that do not preserve __dirname. The problem does not apply to browserify for example, as long as node_modules/<native-addon>/build/Release/*.node is copied alongside the JS bundle.

Mike-Dax commented 3 years ago

I'm not a fan of breaking the static analysis chain of our bundlers, finding the native module at runtime or packaging all the native modules into the npm package itself. I wrote a Yarn v2 that we've been using internally for a while that takes a different approach.

https://github.com/electricui/yarn-prebuilds

It intercepts dependencies on the bindings package (and could conceivably have support added for node-gyp-build in a similar fashion) and rewrites them to be a static export of the native module. The native module is strictly fetched from a prebuilt source however.

I figured doing it at the package management level made more sense than at a Webpack / Babel level.

I'm not sure if this solution is useful to any of you, but I thought I'd drop it in here.

rdwoodring commented 3 years ago

I'm getting hit with this as well on a project at work. I'd be happy to contribute to a fix, with some guidance.

mafintosh commented 3 years ago

We have a tag system already for picking a build with certain features, ie node.abi.42.node to pick abi=42. Tagging with the module name would solve any ambiguity for these cases where the bundlers reorg things, ie node.name.my-native-build.node.

If someone wants to add support for that I'd be happy to help review. It just has be to backwards compat, ie, if no name tag fits pick the one without a name tag etc.

rdwoodring commented 3 years ago

If I can get time, I can take a look this evening. Are you looking for something like what was proposed in https://github.com/prebuild/node-gyp-build/issues/22#issuecomment-504098774? Updating the call to accept an optional name like

require('node-gyp-build')(__dirname, 'leveldown')

and then search for the proper binary?

lgleim commented 3 years ago

While I do not have anything to contribute, we have also encountered this issue recently and have found it extremely hard to debug, especially in conjunction with larger toolchains (in our case tsc -> webpack -> nodegui packer -> linuxdeployqt). The lack of a helpful error message (as raised in https://github.com/prebuild/node-gyp-build/issues/22#issuecomment-504098774) proved to be quite frustrating. In the end we spent a full workday on pinpointing the issue to broken dependency bundling due to node-gyp-build. Especially since many npm packages use node-gyp-build internally, it would be extremely helpful to mention this potential problem directly in the generated error messages, such that developers have an easier time finding the correct place to look for resolutions.

ivan-nemtinov commented 3 years ago

Simple workaround for bundling of a project with multiple native dependant packages is to include the most heavy package to the bundle and do not bundle others. In this case we have this native module in /build/prebuilds/linux-x64/node.abi72.node (for Linux), and mark other native dependant packages in node_modules. This can be done with webpack-node-externals plugin with allowlist setting. Also we can bundle remaining native modules independently and replace non-bundled modules in the main bundle using NormalModuleReplacementPlugin. I think this is much better than giving up bundling.

thegecko commented 2 years ago

Simple workaround for bundling of a project with multiple native dependant packages is to include the most heavy package to the bundle...

Another short-term fix could be to externalise the native packages and allow the commonjs system to pick them up from node_modules at runtime.

https://webpack.js.org/configuration/externals/#root

e.g.:

...
        externals: {
            argon2: 'commonjs argon2',
            leveldown: 'commonjs leveldown'
        }
...