andywer / threads-plugin

📦 Makes your threads.js code build with webpack out of the box.
Apache License 2.0
28 stars 6 forks source link

Issue with Node builtins in worker: ModuleNotFoundError: Module not found: Error: Can't resolve 'child_process' #13

Closed markneub closed 4 years ago

markneub commented 4 years ago

I'm using threads-plugin with Vue CLI and Electron, and forcing threads.js to use Web Workers instead of Node Workers as explained in https://github.com/andywer/threads.js/issues/199. My worker code uses some Node built-in modules, like child_process. When webpack tries to compile my code with a require('child_process') in the worker code, I get the error: ModuleNotFoundError: Module not found: Error: Can't resolve 'child_process'.

I know it should be possible to use node builtins in worker code in my setup, because:

  1. Electron supports this with the nodeIntegrationWorkeroption: https://electronjs.org/docs/tutorial/multithreading
  2. It works fine with https://github.com/nolanlawson/promise-worker and loading the worker using https://github.com/webpack-contrib/worker-loader. Thus, I suspect that something about the way the worker code is loaded with threads-plugin is causing the issue. (I am migrating from promise-worker to get some of the more advanced features of threads.js, like pools and Transfer)

Seems related to https://github.com/andywer/threads-plugin/issues/10, but I'm not using TypeScript so I made a separate issue. Sorry for being a bit light on code examples in this issue, but I'm not sure what would be most relevant to provide. Would it be helpful for me to provide my webpack config? (it's a bit long because it's mostly the Vue CLI config)

I have tried various modifications to the webpack config, including setting externals: { child_process: 'child_process' } and node: false but nothing has helped. Happy to try any suggestions, thanks! :)

markneub commented 4 years ago

It looks like if i set: browser: { child_process: false } in package.json, then it compiles successfully, but the value of child_process in the worker is an empty object, {}.

I've created a minimal repo that shows off the issue: https://github.com/markneub/threadtest

markneub commented 4 years ago

It looks like this is an issue with at least some built in modules other than child_process as well. For example, require('os') has the same problem -- it resolves fine in the main code, but can't be found when placed in the worker.

andywer commented 4 years ago

Damn… I'm also still a bit sick, so I am just having a quick look over the most pressing issues.

I just added the os module to the browser array, too. Let me know if there are more. It's curious that the tests are green, but I guess you might use the Pool whereas it's not used and potentially tree-shaken away in the tests (or just dead-code-eliminated).

markneub commented 4 years ago

Sorry to hear you're still sick — this issue can wait! And I will keep experimenting in the meantime.

I don't know if adding builtins to the browser array is actually fixing anything, because those modules are still not available in the compiled code. It seems like the issue is that require() statements for node builtins in aren't resolving in in code loaded by threads-plugin.

Thank you for providing such a great library, I'm excited to be able to use it when I get this figured out :).

andywer commented 4 years ago

Yeah, something's not working about the threads.js files excluded via the browsers field. The files requiring the node.js stuff should be ignored in the first place…

Thanks! I hope we can get this fixed quickly.

markneub commented 4 years ago

I've managed to get things working by replacing all require() statements in the worker module and its dependencies with eval('require') (like, eval('require')('child_process')) — hideous, I know. This makes webpack ignore them, and when Node sees them, they resolve to correctly to the builtin modules.

In addition to the use of eval(), it's not a great solution because I had to go manually patch some require()s in a Node-only external dependency, so as soon as I update my dependencies it will blow away. But it does shine some more light on the situation and lets me start playing around with threads!

bormisov1 commented 4 years ago

@andywer FYI, I'm experiencing this as well but with TS, so @markneub workaround is not fully applicable in my case — I use import statements instead of require in worker's code

Not sure this could help, but seems this issue is related https://github.com/webpack/webpack/issues/744

andywer commented 4 years ago

@bormisov1 I think the browser field fix in andywer/threads.js#194 should fix your issue. Fix is on its way… 😉

bormisov1 commented 4 years ago

@andywer Do you mean this fix https://github.com/andywer/threads.js/issues/194#issuecomment-576155972? You already suggested it to me in this issue and it helped, but I'm experiencing a different one here. Being more exact I have this error stack:

Module build failed (from ./node_modules/threads-plugin/dist/loader.js):
ModuleNotFoundError: Module not found: Error: Can't resolve 'child_process' in '/home/vlad/Work/desktop-generator-v3/node_modules/detect-libc/lib'
    at /home/vlad/Work/desktop-generator-v3/node_modules/webpack/lib/Compilation.js:925:10
    at /home/vlad/Work/desktop-generator-v3/node_modules/webpack/lib/NormalModuleFactory.js:401:22
    at /home/vlad/Work/desktop-generator-v3/node_modules/webpack/lib/NormalModuleFactory.js:130:21
    at /home/vlad/Work/desktop-generator-v3/node_modules/webpack/lib/NormalModuleFactory.js:224:22
    at /home/vlad/Work/desktop-generator-v3/node_modules/neo-async/async.js:2830:7
    at /home/vlad/Work/desktop-generator-v3/node_modules/neo-async/async.js:6877:13
    at /home/vlad/Work/desktop-generator-v3/node_modules/webpack/lib/NormalModuleFactory.js:214:25
    at /home/vlad/Work/desktop-generator-v3/node_modules/enhanced-resolve/lib/Resolver.js:213:14
    at /home/vlad/Work/desktop-generator-v3/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/home/vlad/Work/desktop-generator-v3/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:13:1)
    at /home/vlad/Work/desktop-generator-v3/node_modules/enhanced-resolve/lib/UnsafeCachePlugin.js:44:7
    at /home/vlad/Work/desktop-generator-v3/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/home/vlad/Work/desktop-generator-v3/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:13:1)
    at /home/vlad/Work/desktop-generator-v3/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/home/vlad/Work/desktop-generator-v3/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:25:1)
    at /home/vlad/Work/desktop-generator-v3/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:67:43

If I add "browser": { "child_process": false }, to detect-libc/package.json, it changes to

Module build failed (from ./node_modules/threads-plugin/dist/loader.js):
ModuleNotFoundError: Module not found: Error: Can't resolve 'child_process' in '/home/vlad/Work/desktop-generator-v3/node_modules/sharp/lib'

And if I add the same to sharp/package.json, the error becomes

Module build failed (from ./node_modules/threads-plugin/dist/loader.js):
ModuleNotFoundError: Module not found: Error: Can't resolve 'constants' in '/home/vlad/Work/desktop-generator-v3/node_modules/fs-constants'

Not even sure if my issue is related to the one I'm posting to, but the first error seems the same as in the first post here

andywer commented 4 years ago

Ohh sorry, of course.

So you still see the child_process error after patching the browser field in the package.json?

bormisov1 commented 4 years ago

@andywer yeah Not sure you noticed, I updated my comment above with a little more detailed description

bormisov1 commented 4 years ago

@andywer Really unsure how your threads-plugin works, but maybe you could specify child_process and sharp as externals somewhere as suggested here... Sorry if my suggestion lacks some understanding :smile:

andywer commented 4 years ago

That sounds like a pretty bad way to go, but I might have just found a solution:

https://github.com/webpack-contrib/worker-loader/issues/95#issuecomment-352856617

bormisov1 commented 4 years ago

@andywer Awesome, thank you! That seems to eliminate both the child_process and sharp packages errors as in my long comment above, but brings the third one related to fs-constants also mentioned in that comment. Wonder how many else will there be if you'll find a way to fix this one as well :smile:

Not sure, but maybe we need to apply something else to workerCompiler inside of threads-plugin/src/loader to fix fs-constants error, as the guy whose comment you referred to mentions in the end of his comment:

it looks like there's quite a lot of other stuff in the WebpackOptionsApply.js that isn't being applied to the child compiler – do any of these also need to be applied to the child compilation, or are they mostly unnecessary?

bormisov1 commented 4 years ago

@andywer This may help. Not sure how though and why the fix in the link helps.

bormisov1 commented 4 years ago

@andywer I did workerCompiler.options = new WebpackOptionsApply().process(compilerOptions, workerCompiler); in threads-plugin/dist/loader.js right after the line with createChildCompiler also having WebpackOptionsApply imported in the beginning, and what I get is fs-constants error eliminated but this warning in terminal Conflict: Multiple assets emit different content to the same filename js/chunk-vendors.js and this error in electron's window console: Uncaught TypeError: Object(...) is not a function. Just as in this issue. So it probably means, that if we find out which exact part of WebpackOptionsApply.js eliminates the fs-constants, we could only have that along with inheritance of externals... I already tried these, didn't help:

  if (workerCompiler.options.externals) {
    workerCompiler.apply(new ExternalsPlugin(
      workerCompiler.options.output.libraryTarget,
      workerCompiler.options.externals
    ));
  }

  workerCompiler.apply(
    new RequireJsStuffPlugin(),
    new ConstPlugin(),
    new RequireContextPlugin(
      compilerOptions.resolve.modules,
      compilerOptions.resolve.extensions,
      compilerOptions.resolve.mainFiles
    )
  );
andywer commented 4 years ago

Thanks for the work you spent on this, @bormisov1! Unfortunately I don't have the time at the moment to debug those weird webpack quirks. This is a high-prio issue, though, for sure!

If you find out anything else, please share it here :)

kalbert312 commented 4 years ago

I was able to get it working (not throughly tested) with my electron setup after making these changes: https://github.com/andywer/threads-plugin/compare/master...kalbert312:master following the lead of: https://github.com/webpack/webpack/blob/master/lib/WebpackOptionsApply.js#L178-L216.

I'm using create-react-app with rescripts. Here's my related config modifier functions:

const applyWorkerHack = (config) => {
    config = appendWebpackPlugin(new ThreadsPlugin({ target: "electron-renderer" }), config);
    return config;
};
//...
const applyElectronHack = (config) => {
    config.node = {
        global: true,
        fs: true,
    };
    config.target = "electron-renderer";
    return config;
};

Might be more that's missing in the node config object but it's promising.

andywer commented 4 years ago

That looks great, @kalbert312! Thanks for sharing.

So the next steps would be merging your commit(s) and documenting to set the target to electron-renderer?

Not sure about the config.node bit. I suppose you allowed the renderer to access node APIs when creating the electron BrowserWindow?

kalbert312 commented 4 years ago

Yes.

    mainWindow = new BrowserWindow({
                //...
        webPreferences: {
            nodeIntegration: true,
            nodeIntegrationInWorker: true,
        },
    });

Now that I slept on it, the ExternalsPlugin should not be there due to https://www.electronjs.org/docs/tutorial/multithreading#available-apis Later today I'm going to test it with just the NodeTargetPlugin on my setup and then create a PR.

kalbert312 commented 4 years ago

Created https://github.com/andywer/threads-plugin/pull/16.