developit / web-worker

Consistent Web Workers in browser and Node.
https://npm.im/web-worker
Apache License 2.0
1.06k stars 55 forks source link

Running through rollup for node causes issues with dynamic require #6

Open rowanwins opened 4 years ago

rowanwins commented 4 years ago

Thanks for a neat little library!

I was attempting at incorporating it into a library of mine which I bundle using rollup and I get an issue

Error: Dynamic requires are not currently supported by rollup-plugin-commonjs

which was related to this line of your library

From what I can tell it looks like it's to do with requiring the URL of the worker?

Anyway just thought I'd flag it as other users of rollup may run into the same issue.

Cheers Rowan

tunnckoCore commented 4 years ago

True. I think we can replace it with dynamic import import(mod);. The whole idea behind that require is evaluation/execution of that mod.

guybedford commented 4 years ago

Short of directly publishing ESM, one pattern I've used to solve this issue before is to use a Promise.resolve(require()) pattern in CommonJS and then have the CJS to ESM transform specifically know it can replace this with a dynamic import.

I've created a Rollup issue for this here - https://github.com/rollup/plugins/issues/341.

developit commented 4 years ago

@rowanwins are you bundling for web via rollup, or for Node? For web none of that code should be processed. It just requires using the node-resolve plugin with the default browser:true option (or mainFields:['browser','module','main']).

rowanwins commented 4 years ago

I was trying to two seperate bundles so I could support both. My browser bundle is working fine, but my node bundle is where I run into the issue.

developit commented 4 years ago

Just looping back here - it doesn't seem like the require() referenced here should actually be processed by Rollup. It's strictly used to inject code into the Worker at runtime, and there's no way for Rollup to statically analyze the require() statement there anyway since its in a different JavaScript context.

@guybedford - I wonder, is there a way to exempt a require() statement from processing by rollup's commonjs plugin? Maybe just switching it to module.require() or global.require()?

guybedford commented 4 years ago

@developit perhaps try eval('require')(...) here?

gerdstolpmann commented 4 years ago

I was recently bundling with webpack, and also ran into this issue. The solve was to use eval as @guybedford suggests.

adjenks commented 2 years ago

I just ran into this issue as well. My error looked like this:

[!] (plugin commonjs--resolver) Error: invalid import "import(mod)". It cannot be statically analyzed. Variable dynamic imports must start with ./ and be limited to a specific directory. For example: import(./foo/${bar}.js). node_modules/web-worker/cjs/node.js Error: invalid import "import(mod)". It cannot be statically analyzed. Variable dynamic imports must start with ./ and be limited to a specific directory. For example: import(./foo/${bar}.js). at dynamicImportToGlob (/node_modules/@rollup/plugin-dynamic-import-vars/dist/index.js:103:11)

Your solution to label set it to browser:true fixed it for me @developit. I had a suspicion that something like that would be the solution, but I wasn't quite sure where the setting was to disable processing of dependencies. Thanks!

adjenks commented 2 years ago

Maybe not, I thought it fixed it but now the code builds but doesn't resolve modules correctly in the browser :(

Addendum: In the end I rolled back the "openlayers" library to a version that doesn't use the "web-workers" library at all. That's my solution for now.

adjenks commented 1 year ago

I still have to lock openlayers to an old version to work around this.

adjenks commented 1 year ago

Okay, I fixed it by adding this to my rollup config plugins section:

        commonjs({
            dynamicRequireTargets: ['**/node_modules/web-worker/cjs/node.js']
        }),