[wasm] Allow empty string module names for JS imports in WASM runtime#128043
[wasm] Allow empty string module names for JS imports in WASM runtime#128043elringus wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adjusts JSImport resolution to treat an empty-string module name as a valid module key, and adds unit coverage to ensure JSImport works when the module name is "".
Changes:
- Update JS import lookup to branch on
js_module_name != null(so""is treated as provided). - Register a JS import set under the empty module name in the test helper.
- Add a new C# JSImport and unit test validating empty-module-name imports.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/mono/browser/runtime/invoke-js.ts | Changes module-name presence check so empty-string module names route through module import resolution. |
| src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/JavaScriptTestHelper.mjs | Registers module imports for "" and provides emptyModuleNameEcho JS implementation. |
| src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/JavaScriptTestHelper.cs | Adds a JSImport binding using an empty module name. |
| src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/JSImportTest.cs | Adds a unit test validating empty-module-name JSImport behavior. |
| if (js_module_name != null) { | ||
| scope = importedModules.get(js_module_name); | ||
| if (WasmEnableThreads) { | ||
| mono_assert(scope, () => `ES6 module ${js_module_name} was not imported yet, please call JSHost.ImportAsync() on the UI or JSWebWorker thread first in order to invoke ${function_name}.`); |
| @@ -46,6 +46,9 @@ public static void ConsoleWriteLine([JSMarshalAs<JSType.String>] string message) | |||
| [JSImport("intentionallyMissingImportAsync", "JavaScriptTestHelper")] | |||
| public static partial Task IntentionallyMissingImportAsync(); | |||
|
|
|||
|
@elringus why this is useful ? |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
It's similar to global/no-namespace in C#. Otherwise we have to use a synthetic alias for the global space, which complicates the codegen. I'm currently monkey patching this in |
Thanks for explanation @elringus I'm closing this because I don't want to maintain the extra scenarios it will bring. Short version: Globals are bad mainly because they scale poorly. They may seem harmless in small code, but in shared environments they increase collision risk, debugging difficulty, and architectural fragility. If you really want to support your case, please create feature request issue and let's see if it can collect enough upvotes. |
Description
Allow
""to be used as a valid JS module name for[JSImport]in the Mono WASM runtime.Previously,
mono_wasm_lookup_js_importused a truthy check forjs_module_name, so[JSImport(..., "")]skipped the module import registry and fell back to global/internal lookup behavior. This meant imports registered withsetModuleImports("", imports)could not be resolved.The runtime now checks
js_module_name != null, preserving existing behavior for omitted module names while allowing the empty string as a real module key.Testing
Added a regression test that registers imports with
setModuleImports("", ...), calls a managed[JSImport(..., "")], and verifies it resolves through the empty-string module entry.