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

Regression in "require" in ES6 modules. Fails to find "ws" module. #23503

Closed
RamiHg opened this issue Jan 27, 2025 · 4 comments · Fixed by #23516
Closed

Regression in "require" in ES6 modules. Fails to find "ws" module. #23503

RamiHg opened this issue Jan 27, 2025 · 4 comments · Fixed by #23516

Comments

@RamiHg
Copy link
Contributor

RamiHg commented Jan 27, 2025

It looks like a recent change to revert a commit that removed uses of require didn't quite fully revert to the old behavior.

I was getting a bizarre error where Node would refuse to find the "ws" module even though I had installed it using npm install ws. I build my code using -sMODULARIZE -sEXPORT_ES6. The error was:

Error: Cannot find module 'ws'
Require stack:
- /noop.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1060:15)
    at Module._load (node:internal/modules/cjs/loader:905:27)
    at Module.require (node:internal/modules/cjs/loader:1127:19)
    at require (node:internal/modules/helpers:112:18)

"ws" was getting imported from libsockfs.js in the listen function:

        var WebSocketServer = require('ws').Server;
        var host = sock.saddr;

The original code that was initially reverted called createRequire like this:

  const { createRequire } = await import('module');
  let dirname = import.meta.url;
  if (dirname.startsWith("data:")) {
    dirname = '/';
  }
  /** @suppress{duplicate} */
  var require = createRequire(dirname);

However, the change that re-introduced the old code instead introduced it as:

  const { createRequire } = await import('module');
  /** @suppress{duplicate} */
  var require = createRequire('/');

I'm not very familiar with Javascript and Node, but after much head-scratching, I tracked it down to that piece of code. If I replace the createRequire with the original version (the one that first checks for "data:"), my code works!

I can create a simple PR to do that fix if needed.

Thanks!

@RamiHg RamiHg changed the title "require" in ES6 modules fails to find "ws" module. Regression in "require" in ES6 modules. Fails to find "ws" module. Jan 27, 2025
@sbc100
Copy link
Collaborator

sbc100 commented Jan 27, 2025

Interesting. I assumed that the parameter to createRequire was only needed for relative imports like ./foo not for system imports like ws. I guess I was wrong. I guess we need some more testing here.

@sbc100
Copy link
Collaborator

sbc100 commented Jan 27, 2025

Just to confirm, are you trying to actually build websocket server using emscripten that runs in node? Or is this code just getting included incidentally somehow?

@RamiHg
Copy link
Contributor Author

RamiHg commented Jan 28, 2025

Thanks for the quick reply! And yes. I am building a websocket server using emscripten. (Well, I'm porting a POSIX socket server).

@sbc100
Copy link
Collaborator

sbc100 commented Jan 28, 2025

The fix seems easier enough, although I'm having trouble being able to test this (because out test code sets NODE_PATH=..., which obviates the need for the fix).

Can you say more about your use case. Are you running your emscripten-generate code both in node and in the browser?

sbc100 added a commit to sbc100/emscripten that referenced this issue Jan 28, 2025
For most uses of `require()` in emscripten this does not matter since
we load mostly system libraries.  However for the `ws` module it is
needed.  This bug was being masked by the fact that we were setting
`NODE_PATH` in our socket test running.  This is no longer needed now
that we run (non-parallel) tests in the emscripten tree (in out/test).

This is followup to emscripten-core#23265 which itself was an attempt to revert 23169.

Fixes: emscripten-core#23503
sbc100 added a commit to sbc100/emscripten that referenced this issue Jan 28, 2025
For most uses of `require()` in emscripten this does not matter since
we load mostly system libraries.  However for the `ws` module it is
needed.  This bug was being masked by the fact that we were setting
`NODE_PATH` in our socket test running.  This is no longer needed now
that we run (non-parallel) tests in the emscripten tree (in out/test).

This is followup to emscripten-core#23265 which itself was an attempt to revert 23169.

Fixes: emscripten-core#23503
sbc100 added a commit to sbc100/emscripten that referenced this issue Jan 28, 2025
For most uses of `require()` in emscripten this does not matter since
we load mostly system libraries.  However for the `ws` module it is
needed.  This bug was being masked by the fact that we were setting
`NODE_PATH` in our socket test running.  This is no longer needed now
that we run (non-parallel) tests in the emscripten tree (in out/test).

This is followup to emscripten-core#23265 which itself was an attempt to revert 23169.

Fixes: emscripten-core#23503
sbc100 added a commit to sbc100/emscripten that referenced this issue Jan 28, 2025
For most uses of `require()` in emscripten this does not matter since
we load mostly system libraries.  However for the `ws` module it is
needed.  This bug was being masked by the fact that we were setting
`NODE_PATH` in our socket test running.  This is no longer needed now
that we run (non-parallel) tests in the emscripten tree (in out/test).

This is followup to emscripten-core#23265 which itself was an attempt to revert 23169.

Fixes: emscripten-core#23503
sbc100 added a commit to sbc100/emscripten that referenced this issue Jan 28, 2025
For most uses of `require()` in emscripten this does not matter since
we load mostly system libraries.  However for the `ws` module it is
needed.  This bug was being masked by the fact that we were setting
`NODE_PATH` in our socket test running.  This is no longer needed now
that we run (non-parallel) tests in the emscripten tree (in out/test).

This is followup to emscripten-core#23265 which itself was an attempt to revert 23169.

Fixes: emscripten-core#23503
@sbc100 sbc100 closed this as completed in 0e9d6f6 Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants