Refactor wrap-rust bindings to use wrap::prelude::*

[krisbitney]

Closes polywrap/rust-wrap-client#237

[cbrzn]

have you contemplated the case where two wraps has a type that has the exact same name? it happens here for example https://github.com/polywrap/wrap-test-harness/blob/master/cases/subinvoke/02-consumer/implementations/rs/lib.rs#L7

[cbrzn]

also i think that it would be super useful if, when updating wraps codegen; we can also update the wrap test harness just to check that it works; wdyt?

[dOrgJelli]

LGTM! This is a breaking change, so I think we should consider how to best handle that. Happy to chat on a call about this.

I think we will need to increase wrap-rust-abi-bindgen@1.0.0 to ...@2.0.0. With the current polywrap CLI we'll need to release a new breaking change there 0.12.0.

A potentially better way to do this is to have the bindgen URI within the polywrap.yaml manifest. Maybe something like:

source:
  module: Cargo.toml
  schema: ./polywrap.graphql
codegen:
  abi_bindgen: wrapscan.io/polywrap/wrap-rust-abi-bindgen@2

[dOrgJelli]

also i think that it would be super useful if, when updating wraps codegen; we can also update the wrap test harness just to check that it works; wdyt?

This for sure!

[krisbitney]

have you contemplated the case where two wraps has a type that has the exact same name? it happens here for example https://github.com/polywrap/wrap-test-harness/blob/master/cases/subinvoke/02-consumer/implementations/rs/lib.rs#L7

I think namespacing solves this already, right?

[krisbitney]

also i think that it would be super useful if, when updating wraps codegen; we can also update the wrap test harness just to check that it works; wdyt?

Great idea

[krisbitney]

LGTM! This is a breaking change, so I think we should consider how to best handle that. Happy to chat on a call about this.

I think we will need to increase wrap-rust-abi-bindgen@1.0.0 to ...@2.0.0. With the current polywrap CLI we'll need to release a new breaking change there 0.12.0.

A potentially better way to do this is to have the bindgen URI within the polywrap.yaml manifest. Maybe something like:

source:
  module: Cargo.toml
  schema: ./polywrap.graphql
codegen:
  abi_bindgen: wrapscan.io/polywrap/wrap-rust-abi-bindgen@2

All of that makes sense. I like the idea of adding a url to the manifest.

I think this is a really small breaking change. I'm not sure a major version increase is necessary. I think this is more of a 1.0.0 -> 1.1.0 change.

When someone updates their wrap, they just need to change this:

pub mod wrap;
pub use wrap::*;

to this:

pub mod wrap;
pub use wrap::prelude::*;

And possibly a few other imports. Everything else works the same. It is still possible to do something like use wrap::module::Module.

What do you think?

[cbrzn]

have you contemplated the case where two wraps has a type that has the exact same name? it happens here for example https://github.com/polywrap/wrap-test-harness/blob/master/cases/subinvoke/02-consumer/implementations/rs/lib.rs#L7

I think namespacing solves this already, right?

I am not sure (I think not if you ask me); that's why I think we should make sure of that in tests

[krisbitney]

have you contemplated the case where two wraps has a type that has the exact same name? it happens here for example https://github.com/polywrap/wrap-test-harness/blob/master/cases/subinvoke/02-consumer/implementations/rs/lib.rs#L7

I think namespacing solves this already, right?

I am not sure (I think not if you ask me); that's why I think we should make sure of that in tests

Okay. I am working on testing everything in the wrap test harness right now and will set up a PR that includes the changes necessary to prepare for the update.

In the example you shared, one of the add_and_increment methods is behind the ImportedInvokeModule namespace. So there is no conflict. This PR doesn't change that.

fn add_and_increment(args: ArgsAddAndIncrement) -> Result<i32, String> {
        Ok(ImportedInvokeModule::add_and_increment(

[dOrgJelli]

All sounds good @krisbitney, although I would say that if this change requires users to "fix" their wraps if they upgrade, then it is indeed a major version increase. But if wrap & wrap::* still work and no rebuild would be required in that scenario, then I can see how this is a feature rather than a breaking change.

[krisbitney]

All sounds good @krisbitney, although I would say that if this change requires users to "fix" their wraps if they upgrade, then it is indeed a major version increase. But if wrap & wrap::* still work and no rebuild would be required in that scenario, then I can see how this is a feature rather than a breaking change.

Gotcha, okay. Using use wrap::* no longer works for the re-exports that have been moved to prelude. I changed the version to 2.0.0.

[cbrzn]

Okay. I am working on testing everything in the wrap test harness right now and will set up a PR that includes the changes necessary to prepare for the update.

In the example you shared, one of the add_and_increment methods is behind the ImportedInvokeModule namespace. So there is no conflict. This PR doesn't change that.

fn add_and_increment(args: ArgsAddAndIncrement) -> Result<i32, String> {
        Ok(ImportedInvokeModule::add_and_increment(

@krisbitney what I mean is that we have two ArgsAddAndIncrement; one being imported normal and the other one being imported using namespace; my thought is that; if we import everything in the prelude would this two ArgsAddAndIncrement conflict? Hope this makes more sense 😄

[krisbitney]

Okay. I am working on testing everything in the wrap test harness right now and will set up a PR that includes the changes necessary to prepare for the update.
In the example you shared, one of the add_and_increment methods is behind the ImportedInvokeModule namespace. So there is no conflict. This PR doesn't change that.

fn add_and_increment(args: ArgsAddAndIncrement) -> Result<i32, String> {
        Ok(ImportedInvokeModule::add_and_increment(

@krisbitney what I mean is that we have two ArgsAddAndIncrement; one being imported normal and the other one being imported using namespace; my thought is that; if we import everything in the prelude would this two ArgsAddAndIncrement conflict? Hope this makes more sense 😄

Okay, I see. I want to emphasize the module structure has not changed. The types that were already being re-exported under wrap::* have just been moved to wrap::prelude::*.

When we import wraps, the bindings will still prepend their namespaces to their type names. For example, AnotherObject in the TestImport wrap will become:

pub struct TestImportAnotherObject {
    pub prop: String,
}

When we look at types from imported wraps in prelude.rs in the Sanity-000 test case, we can see that everything has TestImport or test_import in its name:

pub use super::imported::test_import_object::TestImportObject;
pub use super::imported::test_import_another_object::TestImportAnotherObject;
pub use super::imported::test_import_enum::{
    get_test_import_enum_key,
    get_test_import_enum_value,
    sanitize_test_import_enum_value,
    TestImportEnum
};
pub use super::imported::test_import_enum_return::{
    get_test_import_enum_return_key,
    get_test_import_enum_return_value,
    sanitize_test_import_enum_return_value,
    TestImportEnumReturn
};
pub use super::imported::test_import_env::TestImportEnv;
pub use super::imported::test_import_module::TestImportModule;
pub use super::test_import::TestImport;

In the example you shared, where the root wrap and its import had methods of the same name, there will be no issue because methods are not re-exported. Only the imported module is re-exported.

[krisbitney]

Maybe I misunderstood what I was supposed to do and didn't re-export everything as I was supposed to?

[krisbitney]

Here is how the wrap test harness will change: polywrap/wrap-test-harness#82

[dOrgJelli]

All looks good to me! Also talked to @cbrzn yesterday and he agreed using prelude::* is indeed best practice: https://github.com/search?q=prelude%3A%3A*&type=code

https://chat.openai.com/share/7152e9f6-c685-4c0b-b37b-e0dd8ac04f08