Skip to content

Remove createRequire in favor of ES6 import #23169

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

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 15, 2024

No description provided.

@sbc100 sbc100 force-pushed the remove_createRequire branch from 4dcf866 to ff377d5 Compare December 15, 2024 06:25
@sbc100 sbc100 requested a review from RReverser December 15, 2024 17:47
@sbc100 sbc100 requested a review from kripken December 15, 2024 19:22
@sbc100 sbc100 enabled auto-merge (squash) December 16, 2024 17:52
@sbc100 sbc100 merged commit c2ba38b into emscripten-core:main Dec 16, 2024
28 checks passed
@sbc100 sbc100 deleted the remove_createRequire branch December 16, 2024 19:47
hedwigz pushed a commit to hedwigz/emscripten that referenced this pull request Dec 18, 2024
@kleisauke
Copy link
Collaborator

I think the require() compat function is still needed in some cases.

file:///home/kleisauke/wasm-vips/lib/vips-node.mjs:186
function fd(a,b,c){if(!(0<=a&&3>=a))return 28;if(0===a)a=Date.now();else if(Hi)a=performance.timeOrigin+performance.now();else return 52;D[c>>3]=BigInt(Math.round(1E6*a));return 0}fd.h="iijp";var gd=()=>{};gd.h="v";var jd=a=>x(P(a));jd.h="vp";var kd=()=>{We+=1;throw"unwind";};kd.h="v";var ld=()=>z.length;ld.h="p";var nd=()=>t?require("os").cpus().length:navigator.hardwareConcurrency;nd.h="i";var od=a=>{pi(a)};od.h="vp";
                                                                                                                                                                                                                                                                                                                                      ^

ReferenceError: require is not defined

#if ENVIRONMENT_MAY_BE_NODE
ENVIRONMENT_IS_NODE ? require('os').cpus().length :
#endif

grep-ing through the codebase, I also found:

var rb = require('crypto')['randomBytes'];

var crypto_module = require('crypto');

global.performance ??= require('perf_hooks').performance;

var nodeWorkerThreads = require('worker_threads');

var fs = require('fs');
var vm = require('vm');

etc.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 20, 2024

Oh wow! I guess we don't have tests that cover any of those? Or at least we don't have tests that cover those in combination with EXPORT_ES6.

Lets try to update all those sites rather than being back this pollyfil.

BTW, do you know which of the over usages is the one you are running into?

sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 30, 2024
This change effectively reverts emscripten-core#23169 because it turns out there are
quite a few other places in the codebase that depend on being able
to call `require`.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 31, 2024
This change effectively reverts emscripten-core#23169 because it turns out there are
quite a few other places in the codebase that depend on being able
to call `require`.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 31, 2024
This change effectively reverts emscripten-core#23169 because it turns out there are
quite a few other places in the codebase that depend on being able
to call `require`.
@kleisauke
Copy link
Collaborator

Indeed, I think some test coverage is missing for this. A simple test would be to check whether require( appears in the output JS file, if we proceed with this. It might also cause issues for users who depend on require() in custom --pre-js or --js-library files, but those cases are likely negligible.

BTW, do you know which of the over usages is the one you are running into?

Only the require('os').cpus().length and require('crypto').randomFillSync(view) snippets were causing issues for me.

sbc100 added a commit that referenced this pull request Dec 31, 2024
This change effectively reverts #23169 because it turns out there are
quite a few other places in the codebase that depend on being able to
call `require`.
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 31, 2024

Indeed, I think some test coverage is missing for this. A simple test would be to check whether require( appears in the output JS file, if we proceed with this. It might also cause issues for users who depend on require() in custom --pre-js or --js-library files, but those cases are likely negligible.

Presumably any such users would either:

  1. Not expect their code to work in an ES6 module context
  2. Use createRequire themselves to add require to the ES6 module context.

sbc100 added a commit that referenced this pull request 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 #23265 which itself was an attempt to revert #23169.

Fixes: #23503
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 this pull request may close these issues.

3 participants