Skip to content

build: Add js Cargo feature flag for WASM builds #48

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

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

Conversation

lpahlavi
Copy link

@lpahlavi lpahlavi commented Apr 7, 2025

Previously, the wasm32 target implicitly assumed a browser environment, which caused issues when building for non-browser WASM environments due to the unconditional inclusion of wasm-bindgen.

This commit introduces an explicit 'js' feature flag, making wasm-bindgen and js-sys conditional dependencies. This allows greater flexibility for different WASM execution environments.

Related to #47

Previously, the wasm32 target implicitly assumed a browser environment,
which caused issues when building for non-browser WASM environments due
to the unconditional inclusion of `wasm-bindgen`.

This commit introduces an explicit 'js' feature flag, making `wasm-bindgen`
and `js-sys` conditional dependencies. This allows greater flexibility
for different WASM execution environments.

Related to solana-program#47
@lpahlavi lpahlavi changed the title build: Add 'js' Cargo feature flag for WASM builds build: Add js Cargo feature flag for WASM builds Apr 7, 2025
@lpahlavi
Copy link
Author

lpahlavi commented Apr 7, 2025

@joncinque This is essentially the same as the PR in the solana-sdk repository.

@joncinque
Copy link
Contributor

This one should be easier to merge since it only affects the interface crate in this repo. I think we should move wasm.rs into a new crate, maybe we call it solana-system-interface-wasm-js?

@LucasSte what do you think?

@lpahlavi
Copy link
Author

@joncinque Thanks for looking into this! If it is confirmed that adding a new solana-system-interface-wasm-js crate is the way to go here, should I take the lead on this or would someone from Anza?

@joncinque
Copy link
Contributor

If you have the time, please go for it!

@LucasSte
Copy link

This one should be easier to merge since it only affects the interface crate in this repo. I think we should move wasm.rs into a new crate, maybe we call it solana-system-interface-wasm-js?

@LucasSte what do you think?

Yes, that would be cool. I noticed that in this repo we also mix crate types (cdylib and rlib) for the same crate. I haven't checked why we need both yet, but I wanted them not be used together (it hinders LTO).

@joncinque
Copy link
Contributor

I noticed that in this repo we also mix crate types (cdylib and rlib) for the same crate. I haven't checked why we need both yet, but I wanted them not be used together (it hinders LTO).

Agreed, we definitely need to change that too

@lpahlavi
Copy link
Author

@joncinque @LucasSte I took a stab at doing something similar here as in the solana-sdk repository. Since I obviously cannot have an impl block for SystemInstruction in the new WASM crate but all the exported methods were static, I simply removed the impl block, which I believe should result in the same behaviour as before. I am not a wasm_bindgen expert though so PTAL and confirm that this is indeed correct.

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