Skip to content
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

Provide a way to reject Promise in instantiateWasm #23780

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

FelixNumworks
Copy link

@FelixNumworks FelixNumworks commented Feb 27, 2025

Also fix a typo in parameters name introduced by ec40bc5

Fixes: #23038

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm!

Can we get a test for it though. See test_instantiate_wasm in test_other.py

@FelixNumworks
Copy link
Author

Sorry I didn't manage to run the test suite locally so I have no idea if my test succeeds. I don't have a lot of time to sink into this :/

Also this PR is changing how those tests work so I probably should wait for it.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Hmm, test failures look valid, but I can't see why this change would break stuff

@@ -166,9 +166,7 @@ Other methods

.. js:function:: Module.instantiateWasm

When targeting WebAssembly, Module.instantiateWasm is an optional user-implemented callback function that the Emscripten runtime calls to perform the WebAssembly instantiation action. The callback function will be called with two parameters, ``imports`` and ``successCallback``. ``imports`` is a JS object which contains all the function imports that need to be passed to the WebAssembly Module when instantiating, and once instantiated, this callback function should call ``successCallback()`` with the generated WebAssembly Instance object.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we put Module.instantiateWasm in double backticks here?

Also, can you wrap these lines at 80 columns?

Module['instantiateWasm'](info, (mod, inst) => {
receiveInstance(mod, inst);
Module['instantiateWasm'](info, (inst, mod) => {
receiveInstance(inst, mod);
resolve(mod.exports);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, this needs to be inst.exports

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.

Provide readyPromiseReject to instantiateWasm
2 participants