Plugin API Surface Refactor #879

ianmarmour · 2025-01-21T14:10:57Z

Description of the change

These changes clean up the interfaces around code generation and plugins so that they will be ready to spin off into their own crate without dramatically increasing the API surface of the new crate. The overview of the changes made are below,

Plugin no longer has a Default implementation.
Plugin is now a wrapper around a Vec<u8>.
Plugin now stores it's kind which can only be provided when the plugin-internal feature is set.
Plugin no longer provides a config_schema function to generate the config_schema (this is now created from a plugin instead and will stay within the CLI crate)
Adds feature flag plugin-internal used to disable/enable internal plugin generation related features (V2 Access, JS Config Support, ETC...) should always be enabled for the CLI crate.
Generator's js_runtime_config is now passed in as an optional byte array.

I tried to completely remove the concept of an internal user plugin but I found that I couldn't get rid of the need for a distinction between an external plugin and an internal one without breaking the ability to support using JS configs with internal plugins. I think there's a handful of different approach to solve this but ultimately I feel like the feature-flag gated enums are an equally valid option compared to the suggested boolean with setters.

Why am I making this change?

To lower the API surface of the plugin/code-generation components in preparation for a future split of that logic into a javy-codegen crate based on @jeffcharles request.

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.

…ns when plugin-internal was not set

jeffcharles

Thanks for the work! I understand it takes time to do and I appreciate you taking the time to do it.

That said, I do have concerns about what's in here. I think there was some misunderstanding around the abstractions I would've liked to see for the Plugin. Specifically, I wanted a richer Plugin struct/abstraction for use in the CLI crate and a different and very slimmed down Plugin struct/abstraction for use in the future codegen crate (specifically in the Generator) with additional properties and gated setters on the Generator to preserve behaviour with a very slimmed down plugin abstraction.

Does that make sense?

jeffcharles · 2025-01-21T15:05:45Z

crates/cli/src/codegen/builder.rs

+#[cfg(feature = "plugin-internal")]
+impl CodeGenBuilder {
+    /// Build a [`CodeGenerator`].
+    pub fn build(self, ty: CodeGenType, js_runtime_config: Vec<u8>) -> Result<Generator> {


Can we make js_runtime_config a setter method instead of an argument?

jeffcharles · 2025-01-21T15:05:59Z

crates/cli/src/codegen/builder.rs

-    pub fn wit_opts(&mut self, opts: WitOptions) -> &mut Self {
-        self.wit_opts = opts;
-        self
+    pub fn new(plugin: Plugin, wit_opts: WitOptions, source_compression: bool) -> Self {


Can we keep the existing builder interface please?

jeffcharles · 2025-01-21T15:09:02Z

crates/cli/src/codegen/mod.rs

+#[cfg(feature = "plugin-internal")]
+impl Generator {
+    /// Creates a new [`Generator`].
+    pub(crate) fn new(ty: CodeGenType, js_runtime_config: Vec<u8>, plugin: Plugin) -> Self {


Similar feedback here, can we make this a builder interface instead of having a new function with parameters? That way we can just gate the setter for the js_runtime_config with the Cargo feature instead of having two different versions of new.

jeffcharles · 2025-01-21T15:16:48Z

crates/cli/src/codegen/mod.rs

    /// Generate the starting module.
    fn generate_initial_module(&self) -> Result<Module> {
        let config = transform::module_config();
        let module = match &self.ty {
            CodeGenType::Static => {
+                // Set runtime config to either a valid runtime config or null bytes.
+                let runtime_config = self.js_runtime_config.clone().unwrap_or_default();


Nit: Can we just have the field start as an empty Vec<u8> instead of an Option<Vec<u8>>? That might simplify things a bit.

jeffcharles · 2025-01-21T15:18:19Z

crates/cli/src/commands.rs

-        /// Optional path to Javy plugin Wasm module. Not supported for
-        /// dynamically linked modules. JavaScript config options are also not
-        /// supported when using this parameter.
+        /// Optional path to Javy plugin Wasm module. Required for dynamically


I'm going to open a separate PR with this part of the change since this was an oversight of mine that I should correct ASAP.

jeffcharles · 2025-01-21T15:50:00Z

crates/cli/src/codegen/mod.rs

+                let eval_bytecode_fn_id = if matches!(
+                    self.plugin.kind(),
+                    PluginKind::Internal(InternalPluginKind::V2)
+                ) {


I was expecting something following along the lines of this feedback in #877 (comment):

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.

So this would look more like:

let eval_bytecode_fn_id = if self.linking_v2_plugin {

With a linking_v2_plugin boolean field set to false defined in Generator along with a method that looks like:

#[cfg(feature = "plugin-internal")] pub fn linking_v2_plugin(&mut self, value: bool) { self.linking_v2_plugin = value; }

jeffcharles · 2025-01-21T16:09:15Z

crates/cli/src/codegen/mod.rs

-                if !self.plugin.is_user_plugin() {
+
+                // Only internal plugins expose eval_bytecode function.
+                if matches!(self.plugin.kind(), PluginKind::Internal(..)) {


Similarly here, I was hoping this would look more like:

if self.linking_v2_plugin || self.linking_default_plugin {

with a similar setter for linking_default_plugin that we could gate behind a feature.

jeffcharles · 2025-01-21T16:22:42Z

crates/cli/src/plugins.rs

+/// Represents any valid Javy plugin.
+#[derive(Clone, Debug)]
+pub(crate) struct Plugin {
+    pub(crate) bytes: Vec<u8>,
+    kind: PluginKind,


Instead of this, can we do:

Suggested change

/// Represents any valid Javy plugin.

#[derive(Clone, Debug)]

pub(crate) struct Plugin {

pub(crate) bytes: Vec<u8>,

kind: PluginKind,

/// Javy plugin.

pub struct Plugin(Vec<u8>);

// Represents any valid Javy plugin.

#[derive(Clone, Debug)]

pub(crate) struct CliPlugin {

pub(crate) bytes: Plugin,

kind: PluginKind,

With the idea that the Plugin struct will live in the codegen crate and the CliPlugin will live in the CLI crate the Generator will reference Plugin. Basically I want to keep the PluginKind knowledge entirely in the CLI crate and use a couple feature gated booleans to get Generator to do what it needs to do rather than use a richer plugin abstraction in the generator so it's straightforward for a codegen user to just pass their plugin bytes along without having to worry about PluginKinds.

jeffcharles · 2025-01-21T16:27:59Z

crates/cli/Cargo.toml

 dump_wat = ["dep:wasmprinter"]
+plugin-internal = []


I don't want to introduce the feature gates in this PR but rather when the codegen crate is created. That said, it would be helpful to mark off with PR comments as to which methods would be feature gated.

jeffcharles · 2025-01-21T16:29:16Z

crates/cli/src/js_config.rs

+}
+
+impl ConfigSchema {
+    pub(crate) fn from_plugin(plugin: &Plugin) -> Result<Option<ConfigSchema>> {


I think you can keep this method on CliPlugin, just not the codegen plugin.

ianmarmour added 5 commits January 18, 2025 16:48

Create stronger abstractions around plugin/generator for code.

23f445f

Working version with enums

fca6bbc

Cleaned up various changes as well as Plugin interfaces.

8ddb147

Changed access level for js_runtime_config on generator

ba4a7cf

Adding missing scope to new function for non plugin-v2

cbd80dd

ianmarmour changed the title ~~Generator refactor~~ Plugin API Surface Refactor Jan 21, 2025

Only pass js_runtime_config if plugin-internal is enabled

a22e2de

ianmarmour marked this pull request as ready for review January 21, 2025 14:42

Fixed an issue with CodeGenBuilder not passing in the correct argumet…

9699d9f

…ns when plugin-internal was not set

jeffcharles self-requested a review January 21, 2025 15:03

jeffcharles reviewed Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin API Surface Refactor #879

Plugin API Surface Refactor #879

ianmarmour commented Jan 21, 2025 •

edited

Loading

jeffcharles left a comment

jeffcharles Jan 21, 2025

jeffcharles Jan 21, 2025

jeffcharles Jan 21, 2025

jeffcharles Jan 21, 2025

jeffcharles Jan 21, 2025

jeffcharles Jan 21, 2025

jeffcharles Jan 21, 2025

jeffcharles Jan 21, 2025

jeffcharles Jan 21, 2025

jeffcharles Jan 21, 2025

Plugin API Surface Refactor #879

Are you sure you want to change the base?

Plugin API Surface Refactor #879

Conversation

ianmarmour commented Jan 21, 2025 • edited Loading

Description of the change

Why am I making this change?

Checklist

jeffcharles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianmarmour commented Jan 21, 2025 •

edited

Loading