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

electron - bundling with asar gives 'Cannot find module 0.bundle.worker.js' error #17

Closed dinataranis closed 4 years ago

dinataranis commented 4 years ago

Hi!

I am building an electron application with electron-webpack and electron-builder. I am trying to use threadsjs. When testing in dev mode everything works great, but unfortunately when trying to pack the app (using electron-builder), with the asar option, I am getting the following error: `Cannot find module 0.bundle.worker.js``

I think it is caused by a mismatch in the worker's path. Since packing with asar option causes the entire app to be bundled as a single file, it can't access the worker at runtime.

After searching the web for a bit I found the asarUnpack option (which specifies which files to unpack when creating the asar archive) is probably what we are looking for, but I think it requires changes in threads-plugin to allow worker files to be bundle as asarUnpack. Code example with asarUnpack flag:

"asarUnpack": [
  "dist/main/0.bundle.worker.js",
  "dist/main/0.bundle.worker.js.map"
]

I have reproduced the issue with the basic template, it can be accessed from here. When bundling with asar: false everything works as expected, but without we get the error above.

Bundling with asar: false is strongly not recommended solution, and it really affects the app performance.

We will be thankful if this can be solved.

P.S. I took the dependencies from my current project, but I get the same error, when I use the latest

Best regards.

andywer commented 4 years ago

Hey @dinataranis, thanks for reporting.

How do you think would the solution look like? The threads-plugin cannot set the asarUnpack option as that latter one is in electron-builder.

I could imagine that setting the asarUnpack option manually (could also set it to something generic like dist/main/*.worker.js*, I think) would make the app work. Have you tried yet?

dinataranis commented 4 years ago

Hi, @andywer, sorry for delay. I tried to set asarUnpackmanually as you advised (it saves *.worker.js* files in app.asar.unpacked instead of packing them in app.asar), but in the release mode I still get the same error Cannot find module ...resources\app.asar\dist\main\0.bundle.worker.js If I understand it correctly, threads-plugin changes the paths given anywhere I use new Worker('./relative/path/to/worker) This causes 'threads' to search for my worker in app.asar file. but files cannot be accessed externally within app.asar, so we have to unpack it as you suggested. nevertheless, we still need to notify threads to search in the new path (a.k.a app.asar.unpacked/dist/main/0.bundle.worker.js) For that I think we need your help

andywer commented 4 years ago

I am not yet 100% sure how the solution eventually needs to look like, but I think we need some way to optionally pass a custom worker-path-rewrite function to the threads-plugin, so you can use something like this.

raz-sinay commented 4 years ago

Hi @andywer, I took a look into this problem as well, and I think what you've suggested is the right way to go. something like an options object passed to

new Worker(path, {
    overrideResolvedPath: (resolvedPath:string)=>string;
})

(That's in typescript to make it more clear.)

Just for checking, we have changed the code under implementation.node.js

function resolveScriptPath(scriptPath) {
    // eval() hack is also webpack-related
    const workerFilePath = typeof __non_webpack_require__ === "function"
        ? __non_webpack_require__.resolve(path.join(eval("__dirname"), scriptPath))
        : require.resolve(rebaseScriptPath(scriptPath, /[\/\\]worker_threads[\/\\]/));
    // ~~~~~~~~hardcoded check~~~~~~~~~~~
    return workerFilePath.replace('app.asar', 'app.asar.unpack');
}

And it does find the module, but unfortunately, we get another error:

Cannot find module 'source-map-support/source-map-support.js'

It makes sense since in 0.bundle.worker.js starts like this:

require("source-map-support/source-map-support.js").install(),function(e){var t={};function r(n){if(t[n])return t[n].exports;var o=t[n]={i:n,l:!1,exports:{}};return e[n].call(o.exports,o,o.exports,r),o.l=!0,o.exports}r.m=e,r.c=t,r.d=function(e,t,n){r.o(e,t)||Object.defineProperty(e,t,{enumerable:!0,get:n})},r.r=function(e){"undefined"!=typeof Symbol&&Symbol.toStringTag&&Object.defineProperty(e,Symbol.toStringTag,{value:"Module"}),Object.defineProperty(e,"__esModule",{value:!0})},r.t=function(e,t){if(1&t&&
.... more code

I'm not sure if it's related to threads or threads-plugin though.

Any ideas?

andywer commented 4 years ago

@raz-sinay No, the option should not go into the Worker instantiation, but be passed to the threads-plugin instantiation in the webpack config, so it can rewrite the path at build time.

Might find some time to look into that tonight.

andywer commented 4 years ago

@raz-sinay @dinataranis Can you try to apply this little patch to your node_modules/threads-plugin?

Don't forget to update the dist/threads-plugin.js and set the rewritePath option on new ThreadsPlugin() in the webpack config 😉

diff --git a/src/index.js b/src/index.js
index f944e7c..e5515e8 100644
--- a/src/index.js
+++ b/src/index.js
@@ -31,6 +31,7 @@ export default class WorkerPlugin {

   apply (compiler) {
     let workerId = 0;
+    const rewritePath = this.options.rewritePath || (workerPath => workerPath);

     compiler.hooks.normalModuleFactory.tap(NAME, factory => {
       for (const type of JS_TYPES) {
@@ -74,7 +75,8 @@ export default class WorkerPlugin {
             }

             const loaderOptions = { name: opts.name || workerId + '' };
-            const req = `require(${JSON.stringify(workerLoader + '?' + JSON.stringify(loaderOptions) + '!' + dep.string)})`;
+            const workerPath = rewritePath(dep.string);
+            const req = `require(${JSON.stringify(workerLoader + '?' + JSON.stringify(loaderOptions) + '!' + workerPath)})`;
             const id = `__webpack__worker__${workerId++}`;
             ParserHelpers.toConstantDependency(parser, id)(expr.arguments[0]);
dinataranis commented 4 years ago

Hi, @andywer Thanks for your update. I tried your solution, but I got the same error. As I see in dist/threads-plugin.js we apply the rewritePath function to the worker's relative path instead of absolute

const workerPath = rewritePath('./worker.js');
andywer commented 4 years ago

@dinataranis @raz-sinay Ohh, sh**, you guys are right, of course. Sorry for the confusion…

@dinataranis Can you try monkey-patching implementation.node.js in threads to replace app.asar with app.asar.unpack as Raz did?

@raz-sinay The source-map-support issue seems to be unrelated to threads/threads-plugin. Maybe an npm ls source-map-support explains where it comes from?

dinataranis commented 4 years ago

Hi, @andywer I changed the code in implementation.node.js to replace app.asar with app.asar.unpack and got another error:

Cannot find module 'source-map-support/source-map-support.js.

So I added source-map-support and all it's dependencies to the asarUnpack option in package.json and ... it works!

"asarUnpack": [
  "dist/main/*.worker.js*",
  "node_modules/source-map/**/*",
  "node_modules/source-map-support/**/*",
  "node_modules/buffer-from/**/*",
]

It means, that all node modules which I use in workers files I have to include to asarUnpack option. It will be great if you can fix the workerFilePath path in theradsjs/thereads-plugin modules. Thank you for you help!

andywer commented 4 years ago

@dinataranis Can you try npm install threads@1.3.1-asar.unpack? It should work without any extra options besides the asarUnpack in the package.json. The node.js workers will automatically try the *.asar.unpack path if the resolved worker path contains .asar/ and instantiation failed.

andywer commented 4 years ago

Ping 👉 @dinataranis.

dinataranis commented 4 years ago

Hi, @andywer I have tried the threads@1.3.1-asar.unpack version, and I got the same error Cannot find module '...\resources\app.asar\dist\main\0.bundle.worker.js' You can see it in my updated project here I debugged it a little and in the release mode in the file threads\dist-esm\master\implementation.node.js in Worker constructor if I print the final path string throw resolvedScriptPath I still see the ...resources\app.asar\dist\main\0.bundle.worker.js

constructor(scriptPath, options) {
    const resolvedScriptPath = resolveScriptPath(scriptPath);
    if (resolvedScriptPath.match(/\.tsx?$/i) && detectTsNode()) {
        super(createTsNodeModule(resolvedScriptPath), { eval: true });
    }
    else if (resolvedScriptPath.match(/\.asar\//)) {
        try {
            super(resolvedScriptPath, options);
        }
        catch (_a) {
            // See <https://github.com/andywer/threads-plugin/issues/17>
            super(resolvedScriptPath.replace(/\.asar([/\/])/, ".asar.unpack$1"), options);
        }
    }
    else {
        super(resolvedScriptPath, options);
    }

    throw resolvedScriptPath;

    this.mappedEventListeners = new WeakMap();
    allWorkers.push(this);
}
dinataranis commented 4 years ago

The small update. I think that the problem could be in this condition resolvedScriptPath.match(/\.asar\//) in implementation.node.js file. I have reproduced it in js console and got null

var path = "resources\app.asar\dist\main\0.bundle.worker.js"
path.match(/\.asar\//) 
// null

It is because of the backslashes in the string. If we try to escape them first and only after what make the search, it should work.

var path = "resources\\app.asar\\dist\\main\\0.bundle.worker.js"
path.match(/\.asar[\\,\/]/)
andywer commented 4 years ago

Yeah, you can monkey-patch it for now using .match(/\.asar[\\\/]/). In the .replace() call the regex is correct (it's wrong in a different way, actually), it's wrong in the condition only (don't ask me why…).

andywer commented 4 years ago

Try this commit 😉 https://github.com/andywer/threads.js/pull/226/commits/ab2f3b9bda651504e678e75f50d78734fc606325

andywer commented 4 years ago

I published the latest feature branch commit using a testing tag. Let me know if this one works for you.

npm install threads@1.3.1-asar.unpack.2
dinataranis commented 4 years ago

Hi, @andywer Just a few remarks about 1.3.1-asar.unpack.2 version. When I have installed it

"dependencies": {
    "threads": "1.3.1-asar.unpack.2",
    ...
}

I didn't get your fresh changes, but I just added them manually (changed the match condition to /\.asar[\/\\]/, replace condition to (/\.asar([\/\\])/ and instead of .asar.unpack$1 should be .asar.unpacked$1) and it works as expected!!

andywer commented 4 years ago

@dinataranis Ohhh. That was me being stupid while publishing. Can you give 1.3.1-asar.unpack.3 a try? That one the latest feature branch commit for real.

dinataranis commented 4 years ago

Just a tiny thing, you have forgotten to change .asar.unpack$1 to .asar.unpacked$1

raz-sinay commented 4 years ago

@andywer Was that merged to some version we can use?

andywer commented 4 years ago

Ohh man. I was so busy, I completely forgot. It's merged and published as threads.js v1.4.0 now! 🙌

raz-sinay commented 4 years ago

I think something is wrong with the regex. because I'm still getting: Cannot find module 'path\to\resources\app.asar\dist\main\1.bundle.worker.js'

andywer commented 4 years ago

@raz-sinay To be on the safe side, can you list the following information?

raz-sinay commented 4 years ago

Sure,

When I test the path against the regex /\.asar[\/\\]/ It seems that it doesn't match because the path is given with a single \. I mean: C:\path\to\worker when I escape it to C:\\path\\to\\worker the regex above does find a match. I'm not sure that's the issue, but it might help investigating so i'm mentioning it.

andywer commented 4 years ago

Not sure if it's the regex. I just quickly tried it in a REPL and it matched:

> "C:\\path\\to\\resources\\app.asar\\dist\\main\\1.bundle.worker.js".match(/\.asar[\/\\]/)
[".asar\", index: 24, input: "C:\path\to\resources\app.asar\dist\main\1.bundle.worker.js", groups: undefined]

Did you set the asarUnpack property in the package.json?

raz-sinay commented 4 years ago

I did, also made sure that it was unpacked. Did it work for you even with a single \ ? I mean unescaped path?

raz-sinay commented 4 years ago

@andywer We did some more investigation. found out that:

// node_modules\threads\dist-esm\master\implementation.node.js
 class Worker extends NativeWorker {
        constructor(scriptPath, options) {
            const resolvedScriptPath = resolveScriptPath(scriptPath, (options || {})._baseURL);
            if (resolvedScriptPath.match(/\.tsx?$/i) && detectTsNode()) {
                super(createTsNodeModule(resolvedScriptPath), Object.assign(Object.assign({}, options), { eval: true }));
            }
            else if (resolvedScriptPath.match(/\.asar[\/\\]/)) {
                try {             
                    **** // If I add here throw new Error(), everything works as expected ****
                    super(resolvedScriptPath, options);  // This line doesn't throw error                    
                }
                catch (_a) {
                    // See <https://github.com/andywer/threads-plugin/issues/17>
                    super(resolvedScriptPath.replace(/\.asar([\/\\])/, ".asar.unpacked$1"), options);
                }
            }
            else {
                super(resolvedScriptPath, options);
            }
            this.mappedEventListeners = new WeakMap();
            allWorkers.push(this);
        }
raz-sinay commented 4 years ago

@andywer any news with this one?

andywer commented 4 years ago

@raz-sinay Ohh, sorry.

Did it work for you even with a single \ ? I mean unescaped path?

Don't have a Windows system to test, besides in CI.

I guess we should not catch and then fix the path, but just assume we need to fix it or maybe add an option to control the ASAR path rewriting, but let it default to rewrite.

raz-sinay commented 4 years ago

I guess we should not catch and then fix the path, but just assume we need to fix it or maybe add an option to control the ASAR path rewriting, but let it default to rewrite.

Sounds good.

andywer commented 4 years ago

I opened a follow-up PR andywer/threads.js#236 and published it under a testing tag as threads@1.4.0-resolve-asar-immediately.

@raz-sinay Give it a try – hope that does the job 😉

raz-sinay commented 4 years ago

I opened a follow-up PR andywer/threads.js#236 and published it under a testing tag as threads@1.4.0-resolve-asar-immediately.

@raz-sinay Give it a try – hope that does the job

Works!

jonluca commented 1 year ago

This should probably be reverted (or optional) now that electron natively supports asar's https://github.com/electron/electron/commit/06a00b74e817a61f20e2734d50d8eb7bc9b099f6