Skip to content

Use async/await instead of promises for wasm loading. NFC #23068

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

Merged
merged 3 commits into from
Dec 10, 2024

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 4, 2024

These get lowered away by babel when targetting older engines.

Followup to #23066, with code size savings.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 4, 2024

This change builds upon #23066

@sbc100 sbc100 marked this pull request as draft December 4, 2024 03:46
@sbc100 sbc100 force-pushed the use_await branch 3 times, most recently from 38970d6 to 0c80cd1 Compare December 5, 2024 00:48
@sbc100 sbc100 marked this pull request as ready for review December 5, 2024 00:49
@sbc100 sbc100 requested review from brendandahl and kripken December 5, 2024 00:49
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 5, 2024

view with ignore whitespace

@sbc100 sbc100 force-pushed the use_await branch 6 times, most recently from c00d46a to e763c3d Compare December 7, 2024 19:30
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 8, 2024
Convert asyncLoad from promises to async/await.

This lays the groundwork for more a larger usage in emscripten-core#23068
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 9, 2024
Convert asyncLoad from promises to async/await.

This lays the groundwork for more a larger usage in emscripten-core#23068
@sbc100 sbc100 requested a review from brendandahl December 9, 2024 21:57
sbc100 added a commit that referenced this pull request Dec 9, 2024
Convert asyncLoad from promises to async/await.

This lays the groundwork for more a larger usage in #23068
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 10, 2024

A few minor cleanups here. I finally rememberd to use a followup commit instead of squashing, sorry I didn't do that earlier.

I'm pretty happy with this PR now, I think its fairly straight forward.

@sbc100 sbc100 enabled auto-merge (squash) December 10, 2024 16:52
These get lowered away by babel when targetting older engines.
// (We must clone the response now in order to use it later, as if we
// try to clone it asynchronously lower down then we will get a
// "response was already consumed" error.)
var clonedResponse = (await response).clone();
Copy link
Member

Choose a reason for hiding this comment

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

Is it perhaps more idiomatic to have the await on line 837? (var x = await fetch(..)) like line 779 etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could but then we would have an extra await there in the non USE_OFFSET_CONVERTER path. For the normal path the response can be passed as a Promise of a Response (i.e. we don't need to await it it in the normal path).

@sbc100 sbc100 disabled auto-merge December 10, 2024 21:34
@sbc100 sbc100 merged commit af2da32 into emscripten-core:main Dec 10, 2024
26 of 28 checks passed
@sbc100 sbc100 deleted the use_await branch December 10, 2024 21:35
hedwigz pushed a commit to hedwigz/emscripten that referenced this pull request Dec 18, 2024
…#23104)

Convert asyncLoad from promises to async/await.

This lays the groundwork for more a larger usage in emscripten-core#23068
hedwigz pushed a commit to hedwigz/emscripten that referenced this pull request Dec 18, 2024
…-core#23068)

These get lowered away by babel when targetting older engines.

Followup to emscripten-core#23066, with code size savings.
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.

3 participants