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

feat(api) Merge js-api into api #2489

Merged
merged 10 commits into from
Jul 23, 2021
Merged

feat(api) Merge js-api into api #2489

merged 10 commits into from
Jul 23, 2021

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Jul 23, 2021

Description

This patch takes the entire wasmer-js crate and merges it into the
wasmer crate.

Inside the lib/api/src/ directory, there are 2 new directories:

  1. a new sys directory, which contains the usual wasmer crate
    implementation,
  2. a new directory js, which contains the implementation of
    wasmer-js.

The Cargo.toml file is still compatible. The default feature
fallbacks to sys-default, which enables the sys feature. All
features related to compilers or engines or anything else prior this
patch, activates the sys feature.

Parallel to that, there is a js-default and js features.

The Cargo.toml file is extensively documented to explain what are
dependencies, dev-dependencies, features and other sections related to
sys or to js.

There is a bug with wasm_bindgen_test where it doesn't compile or
look for tests in tests/*/<test>.rs. The hack is to name files
tests/js_<test>.rs. Ugly, but it works.

How to review?

I think browsing https://github.com/wasmerio/wasmer/tree/js-api-into-api/lib/api/src can provide a nice overview.

Then the biggest contribution of this patch is the new Cargo.toml, https://github.com/wasmerio/wasmer/blob/js-api-into-api/lib/api/Cargo.toml. The trick is

#####
#
# # Dependencies and Development Dependencies for `sys`.
#
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]

to avoid conflicts with wasm-bindgen. Dependencies are grouped in 3 categories: Shared, ones for sys, ones for js.

The second biggest contribution is the lib.rs, https://github.com/wasmerio/wasmer/blob/js-api-into-api/lib/api/src/lib.rs. The trick to support wasm-bindgen correctly, is to declare the crate-type programmatically in lib.rs rather than in Cargo.toml as such:

#![cfg_attr(feature = "js", crate_type = "cdylib")]
#![cfg_attr(feature = "js", crate_type = "rlib")]

Then, there is compiler errors that are raised if some features aren't correctly used, like:

wasmer/lib/api/src/lib.rs

Lines 458 to 461 in 9a5a10f

#[cfg(all(feature = "js", not(target_arch = "wasm32")))]
compile_error!(
"The `js` feature must be enabled only for the `wasm32` target (either `wasm32-unknown-unknown` or `wasm32-wasi`)."
);

The rest is only moving files.

Next steps

As soon as this PR is merged, and probably as soon as #2460 is merged, then we could imagine refactoring lib/api/ to remove duplicated code. Now it is possible :-).

This patch takes the entire `wasmer-js` crate and merges it into the
`wasmer` crate.

Inside the `lib/api/src/` directory, there are 2 new directories:

1. a new `sys` directory, which contains the usual `wasmer` crate
   implementation,
2. a new directory `js`, which contains the implementation of
   `wasmer-js`.

The `Cargo.toml` file is still compatible. The `default` feature
fallbacks to `sys-default`, which enables the `sys` feature. All
features related to compilers or engines or anything else prior this
patch, activates the `sys` feature.

Parallel to that, there is a `js-default` and `js` features.

The `Cargo.toml` file is extensively documented to explain what are
dependencies, dev-dependencies, features and other sections related to
`sys` or to `js`.

There is a bug with `wasm_bindgen_test` where it doesn't compile or
look for tests in `tests/*/<test>.rs`. The hack is to name files
`tests/js_<test>.rs`. Ugly, but it works.
@Hywan Hywan added 🎉 enhancement New feature! 🧪 tests I love tests 📚 documentation Do you like to read? 📦 lib-api About wasmer labels Jul 23, 2021
@Hywan Hywan requested a review from syrusakbary July 23, 2021 10:26
@Hywan Hywan self-assigned this Jul 23, 2021
@Hywan Hywan requested a review from MarkMcCaskey as a code owner July 23, 2021 10:26
@Hywan
Copy link
Contributor Author

Hywan commented Jul 23, 2021

bors try

bors bot added a commit that referenced this pull request Jul 23, 2021
@bors
Copy link
Contributor

bors bot commented Jul 23, 2021

try

Build failed:

@Hywan
Copy link
Contributor Author

Hywan commented Jul 23, 2021

bors try

bors bot added a commit that referenced this pull request Jul 23, 2021
@Hywan Hywan mentioned this pull request Jul 23, 2021
11 tasks
@bors
Copy link
Contributor

bors bot commented Jul 23, 2021

try

Build failed:

@Hywan
Copy link
Contributor Author

Hywan commented Jul 23, 2021

bors try

bors bot added a commit that referenced this pull request Jul 23, 2021
@Hywan Hywan force-pushed the js-api-into-api branch from 171d70f to fcc9e74 Compare July 23, 2021 13:06
@bors
Copy link
Contributor

bors bot commented Jul 23, 2021

@syrusakbary syrusakbary merged commit d3930be into js-api Jul 23, 2021
@bors bors bot deleted the js-api-into-api branch July 23, 2021 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 documentation Do you like to read? 🎉 enhancement New feature! 📦 lib-api About wasmer 🧪 tests I love tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants