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

Make public API consistent #4398

Merged
merged 4 commits into from
Jan 29, 2024
Merged

Make public API consistent #4398

merged 4 commits into from
Jan 29, 2024

Conversation

maminrayej
Copy link
Contributor

This PR tries to combine all the suggestions in #4033:

  • By hiding sys, js and jsc behind the backend module, it makes sure all three backends expose the same API.
  • In order to not break the public API, stub structs, traits, ... are provided via the stub module. js and jsc use this module to expose the same API as sys by re-exporting items that they don't have an implementation for.
  • Unfortunately it seems using #[deprecated] with re-exports doesn't work in Rust yet and is a known issue. So I didn't have a way to incorporate this suggestion from the original issue.

Resolves #4033.

@maminrayej maminrayej changed the title make public api consistent using each backend Make public API consistent Jan 9, 2024
@maminrayej maminrayej marked this pull request as draft January 9, 2024 22:34
@syrusakbary
Copy link
Member

By hiding sys, js and jsc behind the backend module, it makes sure all three backends expose the same API.

I don't see how. This PR just repositioning the files, creating a lib/api/src/backend/unsupported.rs (should those types really exist in other platforms?).

@maminrayej
Copy link
Contributor Author

maminrayej commented Jan 11, 2024

I don't see how. This PR just repositioning the files, creating a lib/api/src/backend/unsupported.rs (should those types really exist in other platforms?).

The problem at hand are the items that are exposed as part of lib/api that depend on a specific backend like sys. One example is what Michael encountered. For example Artifact is exposed when sys is enabled so one can use it and then expect it to work, just to find out their project won't build with js or jsc. This is the inconsistency that this PR is trying to solve. From here on out we have two options:

  1. Remove things like Artifact from our api and put it behind a feature gated module like sys. This will make the api exposed by sys consistent with js and jsc, but requires a breaking change. I don't think we can afford it right now.
  2. Add a temporary stub implementation of Artifact for js and jsc. This also makes the api exposed by these backends consistent with each other. It doesn't require a breaking change and is the way Rust std manages OS specific things for example.

This PR tries to implement the second solution. Also, in order to prevent this mistake from happening again in the future, it explicitly re-exports everything from the backend. For example, assume we add Foo and expose it via sys. We have to expose it via backend too. But, since backend expects all of its imps to expose Foo, lib/api won't get compiled until we provide an impl of Foo for all backends, preventing any inconsistency to happen again.

So in conclusion, this PR makes the api exposed by different backends consistent without a breaking change, and prevents inconsistencies in the future.

Note that this PR is marked as draft and isn't still completely ready to be reviewed.

@maminrayej maminrayej marked this pull request as ready for review January 18, 2024 22:16
lib/api/src/lib.rs Outdated Show resolved Hide resolved
@maminrayej maminrayej merged commit 8349271 into master Jan 29, 2024
52 checks passed
@maminrayej maminrayej deleted the make-public-api-consistent branch January 29, 2024 23:55
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.

The wasmer crate's sys, js, and jsc feature flags expose an inconsistent public API
2 participants