Skip to content

Fix WASM binding resolution across module scopes - #154

Open
Arnold-Seb wants to merge 1 commit into
thoth-tech:mainfrom
Arnold-Seb:fix/wasm-binding-loading
Open

Fix WASM binding resolution across module scopes#154
Arnold-Seb wants to merge 1 commit into
thoth-tech:mainfrom
Arnold-Seb:fix/wasm-binding-loading

Conversation

@Arnold-Seb

Copy link
Copy Markdown

Summary

Generated C# WASM bindings were failing to load and initialise correctly at runtime. The lookup in CSharpWasmExpo/main.js that resolves each binding method against the WASM module scope only checked a single naming pattern, so methods exposed under a different scope or naming convention (e.g. __sklib_-prefixed keys) were silently missed.

Change

Updated the binding resolution logic in CSharpWasmExpo/main.js to fall back through multiple lookup strategies when resolving a binding function:

  1. globalThis[name]
  2. wasmScope[name]
  3. wasmScope["_CPP_" + name]
  4. A wasmScope key that starts with __sklib_ and matches the target name

This mirrors how the underlying SplashKit bindings are actually exposed across the different generated/module scopes, rather than assuming a single fixed pattern.

Testing

  • Re-tested binding loading across the CSharpWasm and CSharpWasmExpo projects to confirm bindings now resolve and initialise correctly.
  • Exercised edge-case scenarios (bindings exposed under the __sklib_ prefix and via the iframe-scoped wasmScope) to confirm the fallback resolves them without regressing previously-working lookups.

Scope

This PR only touches CSharpWasmExpo/main.js. No build artifacts or generated files are included.

@jankiluitel jankiluitel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the changes and the updated binding resolution approach looks good. The fallback handling across different module scopes improves robustness while keeping the change focused. The implementation is clear and the existing checks are passing. Approved from my side. ✅

@Rhinoatron Rhinoatron left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work on the change it's focused and fits the follow up runtime stability work described in the task notes. The updated binding resolution logic looks clearer and safer than the previous eval(name) approach. The fallback checks across globalThis, the WASM/iframe scope, _CPP_ names, and __sklib__-prefixed names make sense for improving C# WASM runtime stability and browser execution reliability. I also checked for related PRs. I found #147, which is still open and touches the same C# WASM binding-loading area, but this PR appears to be a more specific and current fix. It may be worth confirming with the mentor whether #147 should be closed or superseded so there is no confusion. Approved.

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.

4 participants