Add missing options.support_node check when using preload cache

[dashodanger]

I was receiving an "Uncaught (in promise) ReferenceError: isNode is not defined" error when using the preload cache and building with sENVIRONMENT=web. This appears to fix the issue and seems to be in line with other checks for options.support_node in the script

[sbc100]

Can we update one of the browser test that use use_preload_cache to specify only web environment?

[pbdot]

Running into this as well, seems like it should respect the node config?

[sbc100]

Running into this as well, seems like it should respect the node config?

Yup, we are just waiting a on test update.

[pbdot]

@sbc100 Thanks, is the test update tracked anywhere?

[sbc100]

I'm not sure what you mean. This PR is almost ready to land I think, its just missing a test. i.e. the problem has already been fixed by @dashodanger, we just need them to add a test and then we can land it.

[dashodanger]

To be honest, I did not realize that the request for a test was directed at me; rather I assumed that if members of the core team/org felt it merited a new test to be added to the suite that they would write one. I've done very little with web/browser content, and even less so with tests.

[pbdot]

I assumed that if members of the core team/org felt it merited a new test to be added to the suite that they would write one

This would be my assumption as well, or at least it would be somehow tracked with an assignee, priority, tag, milestone or other bookkeeping, you know like using any of the project management tools available, which is why I asked.

I guess we'll just keep hitting this significant bug, resting in 1.8k other issues, which has a candidate fix, until whomever we are writes a test, cheers.

[sbc100]

I assumed that if members of the core team/org felt it merited a new test to be added to the suite that they would write one

This would be my assumption as well, or at least it would be somehow tracked with an assignee, priority, tag, milestone or other bookkeeping, you know like using any of the project management tools available, which is why I asked.

I guess we'll just keep hitting this significant bug, resting in 1.8k other issues, which has a candidate fix, until whomever we are writes a test, cheers.

I don't think the snarkiness here is warranted. It very normal for maintainers to request the PRs come with tests.

It sounds like @dashodanger is not comfortable writing tests, which is perfectly fine, but that wasn't expressed until just now.

[kripken]

It does look like there was a miscommunication here. In general we do expect PRs to include tests, and @sbc100 was referring to that, but I do understand that if you are not familiar with that policy then the earlier comments could be unclear as to who should add the test.

Anyhow, now that that's clarified, I think adding a test here is pretty simple, how about this?

diff --git a/test/test_browser.py b/test/test_browser.py
index b16e2481d..33bf3f34f 100644
--- a/test/test_browser.py
+++ b/test/test_browser.py
@@ -613,10 +613,16 @@ If manually bisecting:
       self.skipTest('chrome bug')
     create_file('somefile.txt', '''load me right before running the code please''' + ('_' * extra_size))
     print('size:', os.path.getsize('somefile.txt'))
-    self.compile_btest('main.c', ['--use-preload-cache', '--js-library', 'test.js', '--preload-file', 'somefile.txt', '-o', 'page.html', '-sALLOW_MEMORY_GROWTH'], reporting=Reporting.JS_ONLY)
+    args = ['--use-preload-cache', '--js-library', 'test.js', '--preload-file', 'somefile.txt', '-o', 'page.html', '-sALLOW_MEMORY_GROWTH']
+    self.compile_btest('main.c', args, reporting=Reporting.JS_ONLY)
     self.run_browser('page.html', '/report_result?exit:0')
     self.run_browser('page.html', '/report_result?exit:1')
 
+    # test with ENVIRONMENT=web, to check for problems with node.js support
+    # (see #23059)
+    self.compile_btest('main.c', args + ['-sENVIRONMENT=web'], reporting=Reporting.JS_ONLY)
+    self.run_browser('page.html', '/report_result?exit:0')
+
   def test_preload_caching_indexeddb_name(self):
     self.set_setting('EXIT_RUNTIME')
     create_file('somefile.txt', '''load me right before running the code please''')

That indeed crashes before this PR (it just adds a test for preload caching + ENVIRONMENT=web). If the PR fixes it, then let's add it to the PR (edit: specifically, I mean the PR owner, @dashodanger - can you do that?), and land it.

To test this locally, use

./test/runner browser.test_preload_caching_0* browser.test_preload_caching_1*

[dashodanger]

I just fixed file_package.py from my local emsdk install and then placed that into a cloned repository to make this PR. I haven't actually built emscripten myself, and am not even sure how to properly run the tests you are referring to. I tried the test and commands suggested above with my emsdk install but I get seemingly unrelated errors that I don't really have the time or interest to be running to ground.

[kripken]

Fair enough. Please just add that diff as a commit to this PR then, we can see the test results on CI. Thanks!