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

Incompatible with ESM module (node > 13.2) #220

Closed
PacoDu opened this issue Mar 12, 2020 · 9 comments · Fixed by #257
Closed

Incompatible with ESM module (node > 13.2) #220

PacoDu opened this issue Mar 12, 2020 · 9 comments · Fixed by #257
Labels

Comments

@PacoDu
Copy link

PacoDu commented Mar 12, 2020

Threads.js doesn't work with node ESM module, if "type": "module" is set in the package.json importing threads.js as a dependency, same if main file extension is .mjs.

Worker constructor fails with the following error:

UnhandledPromiseRejectionWarning: Error: Cannot find module 'file:/(...)/add.js'
Require stack:
- (...)/node_modules/threads/dist/master/implementation.node.js
- (...)/node_modules/threads/dist/master/implementation.js
- (...)/node_modules/threads/dist/master/index.js
- (...)/node_modules/threads/dist/index.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:976:15)
    at Function.resolve (internal/modules/cjs/helpers.js:78:19)
    at resolveScriptPath ((...)/node_modules/threads/dist/master/implementation.node.js:63:19)
    at new Worker ((...)/node_modules/threads/dist/master/implementation.node.js:74:40)
    at file:///(...)/test.js:4:35
    at file:///(...)/test.js:10:3
    at ModuleJob.run (internal/modules/esm/module_job.js:110:37)
    at async Loader.import (internal/modules/esm/loader.js:167:24)

Reproduce (requires node > 13.2 and package.json "type":"module"):
test.js

import threads from 'threads'

(async () => {
  const add = await threads.spawn(new threads.Worker("./add.js"))
  const result = await add(1, 1)

  console.log("result:", result)

  await threads.Thread.terminate(add)
})()

add.js

import { expose }  from "threads/worker"

expose({
  add(a, b) {
    return a + b
  }
})

Note: adding extension to worker path or writing worker in commonJS with .cjs extension doesn't change anything.

PS: Thanks for this awesome library !

@andywer
Copy link
Owner

andywer commented Mar 13, 2020

Hey @PacoDu, thanks for reporting!

And add.js truly is located in the same directory that test.js is, I suppose?

Maybe you can put the test repo on GitHub, so it’s easier to quickly try it out.

@andywer andywer added the bug label Mar 13, 2020
@PacoDu
Copy link
Author

PacoDu commented Mar 13, 2020

@andywer Yes, I removed my personal path from the stacktrace but using ls on the path returned by Cannot find module 'file:/(...)/add.js' is successful.

I'll push the test repo for reproduction ;)

@PacoDu
Copy link
Author

PacoDu commented Mar 14, 2020

@PacoDu
Copy link
Author

PacoDu commented Apr 6, 2020

I've updated the repo with some tests and a CI to demonstrate the issue:
https://github.com/PacoDu/threads-esm-bug/blob/master/test.mjs
https://travis-ci.com/github/PacoDu/threads-esm-bug/jobs/315188140

@andywer
Copy link
Owner

andywer commented Apr 8, 2020

Checked it out and could fix a bunch of things (like adding exports to the package.json, removing the file: prefix when resolving the worker path in master/implementation.node.js, …), but it seems as if there are just a few limitations with TypeScript and ES modules in node.js today that are not easy to overcome.

For instance, TypeScript does not allow emitting files with the .mjs extension. Now we could have a post-build script that just renames the files.

That won't work either, though, since node.js in ES module mode seems to require specifying the file extension in every import statement, so we would need to somehow rewrite all import statements in the output code, too… 🙈

I am sorry, but I think that cannot be easily fixed as of today.

@PacoDu
Copy link
Author

PacoDu commented Apr 8, 2020

Ok, thanks for investigating the issue ;)

@kissenger
Copy link

Same issue, is there any workaround?

@andywer
Copy link
Owner

andywer commented May 29, 2020

Hmm, so threads.js doesn't work at all on node.js > 13.2? (Not running node 13/14 myself yet tbh)

@kissenger Can you qickly try to rm node_modules/threads/*.mjs and see if that makes node.js fall back to using the commonjs modules?

@kissenger
Copy link

I need to do the import as follows to avoid getting import error:

import threads from 'threads';
const Worker = threads.Worker;
const Pool = threads.Pool;
const spawn = threads.spawn;

And even after removing the *.mjs files I still get the following error:

Error: Cannot find module 'C:\<my_path>\node_modules\threads\dist\master\file:\C:\<my_path>\nodejs\tests\workers.js'
Require stack:
- C:\<my_path>\nodejs\node_modules\threads\dist\master\implementation.node.js
- C:\<my_path>\nodejs\node_modules\threads\dist\master\implementation.js
- C:\<my_path>\nodejs\node_modules\threads\dist\master\index.js
- C:\<my_path>\nodejs\node_modules\threads\dist\index.js
...
code: 'MODULE_NOT_FOUND',

tbh I reverted my project back to commonJS modules and it works fine. I'm using node v14.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants