-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
JSPI - JSPI_<EXPORTS/IMPORTS> setting and docs update. #21932
Conversation
test/test_other.py
Outdated
@@ -3204,7 +3204,7 @@ def test_embind_return_value_policy(self): | |||
|
|||
def test_jspi_wildcard(self): | |||
self.require_jspi() | |||
self.emcc_args += ['-sASYNCIFY_EXPORTS=async*'] | |||
self.emcc_args += ['-sJSPI_EXPORTS=async*'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only place we use this in the whole test suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are three others. I've left them though so we still test that option until we remove it. I suppose I could just leave one, if that's preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for deprecated settings I'd prefer that all tests use the new thing.. and we have a single test for the deprecated one (can be version of an existing test).
Can you add settings that are actually deprecated to |
- Replace `ASYNCIFY_IMPORTS` and `ASYNCIFY_EXPORTS` with `JSPI_IMPORTS` and `JSPI_EXPORTS`. - Better document JSPI support.
what exports will become async based on what could potentially call an | ||
an async import (``ASYNCIFY_IMPORTS``). However, with JSPI, the async imports | ||
and exports must be explicitly set using ``JSPI_IMPORTS`` and ``JSPI_EXPORTS`` | ||
settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not possible to compute exports in the JSPI case so that users don't need to specify them explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible and we started down that path in this PR. I think the big difference with JSPI, is all "JSPI'ed" exports return a promise even if they don't ever suspend. If we do the auto approach that could lead to exports returning a promise where it's not expected. I think this worked okay with asyncify since often the export didn't actually suspend, so the library code worked as expected.
We could do some hybrid approach, where we check to see what exports we think will suspend and if they're not in the list error out or give the user a list of suspending exports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks, makes sense.
ASYNCIFY_IMPORTS
andASYNCIFY_EXPORTS
withJSPI_IMPORTS
andJSPI_EXPORTS
.