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

Add embed option to polywrap codegen #1893

Merged
merged 7 commits into from
Oct 2, 2023
Merged

Conversation

krisbitney
Copy link
Contributor

@krisbitney krisbitney commented Sep 7, 2023

This PR adds a --embed option to polywrap codegen. When the option is specified, code for embedded wraps will be generated.

See Rust implementation: polywrap/wrap-abi-bindgen#107

Closes #1703

Copy link
Contributor

@dOrgJelli dOrgJelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor NIT, other than this looks great!

packages/cli/src/commands/codegen.ts Show resolved Hide resolved
@dOrgJelli
Copy link
Contributor

Not sure why CI is not green though, need to get that fixed.

Copy link
Collaborator

@namesty namesty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a scalibility concern if there are many embedded wraps/large amounts of data passed into the gateway where the bindgen wraps are hosted?

Not saying we should worry about it now. Just askin' if it makes sense to ocnsider it for the future

@cbrzn
Copy link
Contributor

cbrzn commented Sep 12, 2023

Not sure why CI is not green though, need to get that fixed.

it seems that the rebuild of the wraps is broken due to a wrong path (weird, right?) I am debugging that in #1890

@krisbitney
Copy link
Contributor Author

Should there be a scalibility concern if there are many embedded wraps/large amounts of data passed into the gateway where the bindgen wraps are hosted?

Not saying we should worry about it now. Just askin' if it makes sense to ocnsider it for the future

This is a great point. In the future, if this feature becomes more important, we can build it out differently. For example, we could generate bindings for one embedded wrap at a time.

@dOrgJelli dOrgJelli merged commit 587b147 into origin-dev Oct 2, 2023
8 of 9 checks passed
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.

Support embedding wraps in JS via polywrap embed for app/ projects
4 participants