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

Ability to combine two import objects #258

Closed
tprk77 opened this issue Mar 9, 2019 · 5 comments · Fixed by #286
Closed

Ability to combine two import objects #258

tprk77 opened this issue Mar 9, 2019 · 5 comments · Fixed by #286
Assignees
Labels
🎉 enhancement New feature!

Comments

@tprk77
Copy link

tprk77 commented Mar 9, 2019

Motivation

I should be able to combine two import objects from different origins. For example, I should be able to mix my own functions into the Emscripten import object.

Proposed solution

See also PR #190.

@tprk77 tprk77 added the 🎉 enhancement New feature! label Mar 9, 2019
@bjfish
Copy link
Contributor

bjfish commented Mar 9, 2019

@lachlansneff What would you think about representing imports object during instantiation with something like the following?
Vec<(String, String, &Export)> or a struct instead of a tuple. I used &Export here so that the user code can own the export (not move it) and we can clone it during instantiation.

If there were duplicate module/name pairs that would result in an error.

@tprk77
Copy link
Author

tprk77 commented Mar 12, 2019

I want to apologize for my somewhat curt issue description. I was just a bit disappointed that my PR was not going to get merged. But I don't have the same insight into the project, so it's very understandable if there's another solution in mind. (It's your project after all. 😁)

To illustrate the functionality I'm looking for, it might help to show my usage:

https://github.com/tprk77/experiments_in_ros2_rust_and_wasm

Specifically, these lines:

https://github.com/tprk77/experiments_in_ros2_rust_and_wasm/blob/2fce069622477fdb795d839349185650019ccb63/ros2_ws/src/ros2_rust_wasm/src/main.rs#L54-L65

I have two versions of the same "app," one in Rust and one in C++. I want to expose the same API imports to both. But to get the C++ app working, I also need the Emscripten imports. (ros::get_imports() is returning an ImportObject containing a ROS2-like API.)

The actual mechanism doesn't really matter to me, but it would be nice if I could still use the imports! macro because it's so convenient. My concern at the moment is with Emscripten, but I think there's also a more general issue of what to do when there are multiple import objects corresponding to multiple APIs. For example, if the mod sensors supplies an import object for a Sensor API, while the mod motors supplies an import object for the Motors API, how to we use both in the same instance?

@lachlansneff
Copy link
Contributor

@tprk77, we're really sorry about not being responsive enough with your other issue. Things have been quite busy here. I'll make sure to take a look at this pr tomorrow morning.

@MarkMcCaskey MarkMcCaskey self-assigned this Mar 18, 2019
This was referenced Mar 19, 2019
bors bot added a commit that referenced this issue Mar 25, 2019
286: Add extend to imports  r=MarkMcCaskey a=MarkMcCaskey

a rewrite of #283 with a focus on simplicity

resolves #258

Co-authored-by: Mark McCaskey <[email protected]>
@bors bors bot closed this as completed in #286 Mar 25, 2019
@tprk77
Copy link
Author

tprk77 commented Mar 28, 2019

I just updated my project to use the new extend. Very nice, seems to be working great. 🎉

@MarkMcCaskey
Copy link
Contributor

That's great! We're glad to hear that.

Thanks for your PR by the way, it helped me get adjusted to the Wasmer code base! Best of luck with your project and let us know if there's anything we can do for you to make using Wasmer better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 enhancement New feature!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants