Add test for ES6 + pthread + node #20939

kripken · 2023-12-18T20:54:07Z

This combination was not tested before.

In fact even running ES6 in node was not tested before (only the browser was).

kripken · 2023-12-18T20:56:01Z

test/test_other.py

+    # parallel to the code we emit for HTML in ScriptSource.replacement).
+    create_file('runner.mjs', '''
+      import initModule from "./hello.mjs";
+      initModule();


I am unsure why this initModule pattern is needed. It is already in use in the codebase as the comment mentions, and it works, so it looks correct. But oddly I can't find an initModule method anywhere in our codebase or in the node or JS docs..?

@sbc100 @RReverser maybe you know?

Oh, I guess this is just executing the default export from the module? What we used to call "MODULARIZE_INSTANCE"...

The default export is called Module.. not initModule and it should return a promise of a module so you would need to await on it.

@kripken is correct. The actual name you bind the default export to doesn't matter / doesn't have to match between import and export sides.

We should perhaps unify this method across all JS module testing we do here in test_core.py or test_other.py.

How about maybe a new self.run_js_module('hello.mjs') that know how to run emscripten-built js modules? (i.e. takes are of building the runner.mjs).

@sbc100 Do we test this in core? All I found is testing in the browser, and some compilation-only tests in other. This is the first non-browser test to actually run EXPORT_ES6 code AFAICT.

With that said I like the idea to add run_js_module but it would only be used in this one function for now?

If this is the only place we test EXPORT_ES6 so far in test_core.py then maybe just leave it as is and we can refactor later..

I'm a little surprised that it is!

Yeah, I was surprised too...

My guess is that we couldn't add ES6 testing when our node version was too old. Then we updated node but didn't add testing. But now we can 🚀

In that case maybe just call this test_export_es6 and have a pthread variant of it?

Good idea, done.

pca006132 · 2024-01-01T13:05:01Z

the test will fail if you add -s PTHREAD_POOL_SIZE=4 to the parameters:

ReferenceError [Error]: require is not defined in ES module scope, you can use import instead

#17664 (comment)

kripken · 2024-01-02T23:49:16Z

@pca006132 do you mean the new test added in this PR? I added the flag you mentioned to it but it still passes for me. Were you adding the flag in some other way?

$ git diff
diff --git a/test/test_other.py b/test/test_other.py
index b0ff5f242..fda6b3958 100644
--- a/test/test_other.py
+++ b/test/test_other.py
@@ -402,7 +402,7 @@ class other(RunnerCore):
 
   @parameterized({
     '': ([],),
-    'pthreads': (['-pthread'],),
+    'pthreads': (['-pthread', '-sPTHREAD_POOL_SIZE=4'],),
   })
   def test_export_es6(self, args):
     self.run_process([EMCC, test_file('hello_world.c'), '-sEXPORT_ES6',

$ ./test/runner other.test_export_es6_pthreads
Test suites:
['test_other']
Running test_other: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
test_export_es6_pthreads (test_other.other.test_export_es6_pthreads) ... ok

----------------------------------------------------------------------
Ran 1 test in 1.188s

OK

pca006132 · 2024-01-03T01:48:33Z

I run it manually, but I think the major difference is that I have a package.json in the current directory with {"type":"module"}.

Commands I used:

echo "int main() { return 0; }" > hello-world.c
emcc -pthread hello-world.c -s PTHREAD_POOL_SIZE=4 -o hello-world.mjs
echo "import Module from './hello-world.mjs'; Module();" > test.mjs
echo '{"type":"module"}' > package.json
node test.mjs

kripken · 2024-01-04T01:00:29Z

@pca006132 I see, thanks. With those steps I get this error in node 18:

This file is being treated as an ES module because it has a '.js' file
extension and '/tmp/NOW/package.json' contains "type": "module".
To treat it as a CommonJS script, rename it to use the '.cjs' file extension.

I'm actually not very familiar with the module ecosystem in JS, but from that error message I take it that this is some kind of conflict between the package.json file and the .js extension. Can you perhaps make thepackage.json file not make that claim about the .js file somehow? (I mean, does it have to modify the meaning of all the files in the directory?)

sbc100 · 2024-01-04T01:27:33Z

I think that warning is harmless since in this case you do want to treat the file as a module, right?

Best to output to a file that ends in .mjs in this case I think.. I assume you didn't do that @kripken ?

pca006132 · 2024-01-04T05:15:48Z

It is not a warning, but an error. There is no difference in whether the file ends with .mjs or .js when package.json contains "type": "module". I think the error is just due to require not being redefined, if we add

const { createRequire: createRequire } = await import("module");
var require = createRequire(import.meta.url);

(code from the generated hello-world.mjs file) it works. Note that also need to replace __filename with import.meta.url because __filename is not defined in ES modules.

I think "type": "module" in package.json cannot be easily removed in some projects, this change the meaning of every .js files. By default they are CommonJS, and this makes them ES module. It is not desirable to require users modify this when we have a simple solution.

And yes it is better to output *.worker.js as *.worker.mjs when the extension of the main file is .mjs.

kripken · 2024-01-04T18:32:30Z

@pca006132 Interesting, looks like we have related logic here:

emscripten/src/shell.js

Lines 197 to 201 in 672bc1f

    
           #if EXPORT_ES6 && ENVIRONMENT_MAY_BE_WEB 
        
             const { createRequire } = await import('module'); 
        
             /** @suppress{duplicate} */ 
        
             var require = createRequire(import.meta.url); 
        
           #endif

cc @kleisauke who added that quoted code, and also @RReverser who I believe knows this stuff, hopefully together we can find a good plan here (I know little of it myself, I'm afraid).

RReverser · 2024-01-04T19:26:29Z

looks like we have related logic here:

The problem seems to be that we don't have the same logic for worker.js which still unconditionally uses require. It would need same patches as we have in the main JS for __filename, static imports and require.

sbc100 · 2024-01-04T19:49:09Z

Would it help if (for the time being anyway) we gave our worker.js the .cjs extension?

RReverser · 2024-01-04T22:26:31Z

Would it help if (for the time being anyway) we gave our worker.js the .cjs extension?

Maybe, worth a try. TBH until this report I didn't even know that Node.js looked at extensions of workers too.

kripken · 2024-01-09T00:32:29Z

Thanks for the tips, an attempt at a fix is in #21041. That seems to resolve things with such a package.json file.

This file can be an ES6 module if a package.json file indicates that all files in the directory are. We need to apply the same tricks as we apply to the main JS file in that case, with createRequire etc. We also need to emit the suffix .worker.mjs and not .js, or else node will error. Followup to #20939 and fixes the package.json discussion after that PR landed.

Add test for ES6 + pthread + node

eaef143

kripken requested a review from sbc100 December 18, 2023 20:54

kripken commented Dec 18, 2023

View reviewed changes

kripken mentioned this pull request Dec 18, 2023

emscripten tbb stuff elalish/manifold#653

Closed

refactor

8813f2b

sbc100 approved these changes Dec 19, 2023

View reviewed changes

kripken enabled auto-merge (squash) December 19, 2023 00:24

kripken merged commit 0161d35 into emscripten-core:main Dec 19, 2023
27 checks passed

kripken deleted the node_es6_pthread branch December 19, 2023 01:00

kripken mentioned this pull request Jan 9, 2024

[Pthreads] Fix worker.js in ES6 module environments #21041

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for ES6 + pthread + node #20939

Add test for ES6 + pthread + node #20939

kripken commented Dec 18, 2023

kripken Dec 18, 2023

kripken Dec 18, 2023

sbc100 Dec 18, 2023

RReverser Dec 18, 2023

sbc100 Dec 18, 2023

kripken Dec 18, 2023

sbc100 Dec 18, 2023

kripken Dec 18, 2023

sbc100 Dec 18, 2023

kripken Dec 18, 2023

pca006132 commented Jan 1, 2024

kripken commented Jan 2, 2024

pca006132 commented Jan 3, 2024

kripken commented Jan 4, 2024

sbc100 commented Jan 4, 2024

pca006132 commented Jan 4, 2024

kripken commented Jan 4, 2024

RReverser commented Jan 4, 2024

sbc100 commented Jan 4, 2024

RReverser commented Jan 4, 2024

kripken commented Jan 9, 2024

Add test for ES6 + pthread + node #20939

Add test for ES6 + pthread + node #20939

Conversation

kripken commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pca006132 commented Jan 1, 2024

kripken commented Jan 2, 2024

pca006132 commented Jan 3, 2024

kripken commented Jan 4, 2024

sbc100 commented Jan 4, 2024

pca006132 commented Jan 4, 2024

kripken commented Jan 4, 2024

RReverser commented Jan 4, 2024

sbc100 commented Jan 4, 2024

RReverser commented Jan 4, 2024

kripken commented Jan 9, 2024