Plugin API Surface Refactor #879
Conversation
There was a problem hiding this comment.
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?
crates/cli/src/js_config.rs
Outdated
| } | ||
|
|
||
| impl ConfigSchema { | ||
| pub(crate) fn from_plugin(plugin: &Plugin) -> Result<Option<ConfigSchema>> { |
There was a problem hiding this comment.
I think you can keep this method on CliPlugin, just not the codegen plugin.
ianmarmour
force-pushed
the
generator-refactor
branch
from
4aaa655 to
ce388f5
Compare
There was a problem hiding this comment.
I couldn't see a use case where this was required over just implementing the generator itself as a builder pattern.
crates/cli/src/codegen/mod.rs
Outdated
| /// Set true if linking with a default plugin module. | ||
| pub fn linking_default_plugin(&mut self, value: bool) -> &mut Self { | ||
| self.plugin_kind = if value { | ||
| plugin::PluginKind::Default | ||
| } else { | ||
| plugin::PluginKind::None | ||
| }; | ||
|
|
||
| self | ||
| } | ||
|
|
||
| /// Set true if linking with a V2 plugin module. | ||
| pub fn linking_v2_plugin(&mut self, value: bool) -> &mut Self { | ||
| self.plugin_kind = if value { | ||
| plugin::PluginKind::V2 | ||
| } else { | ||
| plugin::PluginKind::None | ||
| }; | ||
|
|
||
| self | ||
| } | ||
|
|
||
| /// Runtime configuration options to pass to the module. | ||
| pub fn js_runtime_config(&mut self, js_runtime_config: Vec<u8>) -> &mut Self { | ||
| self.js_runtime_config = js_runtime_config; | ||
| self | ||
| } | ||
| } |
There was a problem hiding this comment.
These will all sit behind the feature flag.
crates/cli/src/codegen/mod.rs
Outdated
| let import_namespace = match self.plugin_kind { | ||
| plugin::PluginKind::V2 => "javy_quickjs_provider_v2".to_string(), | ||
| _ => self.plugin.import_namespace()?, | ||
| }; |
There was a problem hiding this comment.
So this kind of sucks... I'll admit it... but I couldn't think of a better way to keep the import_namespace function in this crate while supporting the existing behavior/tests.
There was a problem hiding this comment.
Could you add a function like plugin_kind.import_namespace(&plugin) -> String to PluginKind with this implementation? It's moving logic around but at least that way the namespace will be in the plugin module as opposed to the generator.
There was a problem hiding this comment.
I like that quite a bit more.
crates/cli/src/codegen/plugin.rs
Outdated
|
|
||
| /// Uses the plugin to generate QuickJS bytecode. | ||
| impl Plugin { | ||
| /// Uses a plugin to generate QuickJS bytecode. | ||
| pub fn compile_source(&self, js_source_code: &[u8]) -> Result<Vec<u8>> { |
There was a problem hiding this comment.
This probably doesn't matter quite yet but do we want/need this to be public? Do we not just want the Generators generate method to be public instead?
There was a problem hiding this comment.
Yeah a bunch of the methods that were public probably should be changed to pub(super) or pub(crate).
|
Actually I do need you to run |
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,
Pluginis now a wrapper around aVec<u8>.Generatornow storesplugin_kindinternally to determine if it's linking to a User/Default/V2Plugin.Generatornow exposeslinking_v2_pluginorlinking_default_pluginthat can have their visibility controlled with a future feature flag to support the existingcliuse cases.codegencrate rather thencli.CliPluginwrapper that storesPluginKindinformation for use with config generation logic in thecli.CodeGenBuilderwas removed as it's behavior is provided byGenerator.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-codegencrate based on @jeffcharles request.Checklist
javy-cliandjavy-plugindo not require updating CHANGELOG files.