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

Split code generation logic into javy-codegen crate #877

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ianmarmour
Copy link

Description of the change

This change accomplishes the following,

  1. Separates codegen logic out into it's own crate javy-codegen
  2. Updates javy-codegen to use DI for the required WASM plugins (providers).

Outside of that all existing behaviors should continue to function as is as far as I can tell.

Why am I making this change?

To enable runtime generation of WASM in a micro-service via direct consumption of the javy-codegen crate.

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-plugin do not require updating CHANGELOG files.
  • I've updated the relevant crate versions if necessary. Versioning policy for library crates
  • I've updated documentation including crate documentation if necessary.

@ianmarmour ianmarmour marked this pull request as ready for review January 14, 2025 03:28
@jeffcharles
Copy link
Collaborator

First of all, thanks for making this PR!

My main concern with the changes in the current state of this PR is a lot of internal implementation details would be exposed as part of the public API on the codegen crate. Specifically around things like that there are v2 and default plugins and JsConfigs. Ideally we avoid exposing those implementation details so the crate is both easier to use and to maintain going forward. I think there's some changes that would need to be made to some of the existing Plugin and Generator abstractions to avoid this. Ideally in a separate PR that the codegen work could be based off of.

More concretely what I'm thinking is:

  • I'm assuming anyone other than the Javy CLI consuming the codegen crate will be using a user plugin (even if that user plugin happens to be the default plugin).
  • Update Generator to no longer reference Plugin but instead reference a wrapper type around a byte slice and have methods that set booleans on self for whether it's linking to the v2 plugin or linking to the default plugin. Then the codegen crate could have a non-default Cargo feature that gates access to those methods so non-CLI consumers don't have to be aware of the different types of plugins. compile_source and import_namespace could be changed to implemented as functions that take the wrapper plugin type and referenced statically.
  • Update Generator to expose a method to set a byte slice to use for the js_runtime_config instead of taking a js_runtime_config when creating the generator. We could also gate access to this method behind the same Cargo feature outlined above. This would mean the JSConfig could stay in the CLI crate and non-CLI consumers aren't aware of it. If consumers want to use those configs, I would recommend creating a plugin that sets the configs instead.

I think with those changes, it would significantly reduce the number of concepts exposed by a codegen crate API (assuming the Cargo feature exposing the configurations for configs, v2 plugins, and default plugins is left in the default disabled state).

How does that sound?

@ianmarmour
Copy link
Author

Thanks for the quick turnaround on feedback!

I'm assuming anyone other than the Javy CLI consuming the codegen crate will be using a user plugin (even if that user plugin happens to be the default plugin).

I think that's a fair assumption to make. I imagine most customers would start by taking a dependency on the plugin crate and codegen and then use more specific user plugins if necessary.

My main concern with the changes in the current state of this PR is a lot of internal implementation details would be exposed as part of the public API on the codegen crate.

Totally in agreement, I wasn't sure what was ideal for the external API and figured there would be feedback around that.

Update Generator to no longer reference Plugin but instead reference a wrapper type around a byte slice and have methods that set booleans on self for whether it's linking to the v2 plugin or linking to the default plugin. Then the codegen crate could have a non-default Cargo feature that gates access to those methods so non-CLI consumers don't have to be aware of the different types of plugins. compile_source and import_namespace could be changed to implemented as functions that take the wrapper plugin type and referenced statically.

I think this makes sense. Is the intent with generator that if the feature flag is set for V2 that the customer would always be linking to V2? Or should this be something that the customers can provide directly to the generator?

Update Generator to expose a method to set a byte slice to use for the js_runtime_config instead of taking a js_runtime_config when creating the generator. We could also gate access to this method behind the same Cargo feature outlined above. This would mean the JSConfig could stay in the CLI crate and non-CLI consumers aren't aware of it. If consumers want to use those configs, I would recommend creating a plugin that sets the configs instead.

Seems fine to me also I'm not sure how often customs will want to set those configs though. I probably won't have time to take a swing at this till the weekend but I'll open another PR to implement just those changes and circle back on this afterwards.

@jeffcharles
Copy link
Collaborator

I imagine most customers would start by taking a dependency on the plugin crate and codegen and then use more specific user plugins if necessary.

We don't publish the plugin crate and we don't intend to. Someone can use javy emit-plugin to get the plugin Wasm file though. We do publish the javy-plugin-api crate and it's pretty trivial IMO to use that to build a basic plugin with a default configuration.

Is the intent with generator that if the feature flag is set for V2 that the customer would always be linking to V2? Or should this be something that the customers can provide directly to the generator?

I was thinking more that the Cargo feature on the codegen crate would gate access to setter methods to set booleans on Generator to say whether the generator is using the default plugin or the v2 plugin. Ideally the Javy CLI is the only user of the codegen crate that would enable that flag and other users wouldn't. Basically, we need something to replace the calls to plugin.is_user_plugin() and plugin.is_v2_plugin() that are used in Generator and defining a couple booleans on the struct with setter methods seems like a decent enough approach.

Seems fine to me also I'm not sure how often customs will want to set those configs though.

Ideally consumers never set those configs. The only reason that support is there is to support using the default plugin with the Javy CLI. The approach I would recommend for anyone doing this programmatically would be to create a plugin with the equivalent configuration options set in the plugin.

@ianmarmour ianmarmour mentioned this pull request Jan 21, 2025
3 tasks
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.

2 participants