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

allow for loading wasm without unsafe wasm execution #1793

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cramt
Copy link

@cramt cramt commented Mar 2, 2025

fixes #1767, by allowed you to not bundle the wasm as a base64 encoded string which is dynamically loaded at eval time

Copy link

vercel bot commented Mar 2, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
yoga-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 2, 2025 7:36pm

@facebook-github-bot facebook-github-bot added CLA Signed Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Mar 2, 2025
"SHELL:--no-entry"
"SHELL:-s ALLOW_MEMORY_GROWTH=1"
"SHELL:-s ASSERTIONS=0"
"SHELL:-s DYNAMIC_EXECUTION=0"
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I learned in the other threads, DYNAMIC_EXECUTION=0 might be making the bindings quite a bit slower, if we still end up using it anyways.

Might be worth removing this argument from the base64 variant, or seeing what happens if we add EMBIND_AOT to both (might require bumping emsdk here

const emsdkVersion = '3.1.28';
)

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/binaries)
add_link_options(
${COMPILE_OPTIONS}
"SHELL:--closure 1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we avoid duplicating the bits here that are not different?

@NickGerleman
Copy link
Contributor

NickGerleman commented Mar 5, 2025

Thanks for digging into this issue!

I've been a little bit weary towards wanting many flavors. E.g. some folks have wanted CommonJS, others have wanted an ASM.js version, not to mention the sync compile vs async compile delineation we have right now. But it seems like this issue is pretty commonly hit, so this makes sense to me.

When we did have multiple flavors though, for sanity, we ran Jest tests against each of them. That would be great to add back here (some of it was in the before state for) ef1d772

I haven't thought about it closely, but I almost wonder if it might make sense to have different packages for these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request to Remove or Address unsafe-eval Usage in Yoga Layout
3 participants