-
Notifications
You must be signed in to change notification settings - Fork 108
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
Change JS config to use a schema and JSON #794
base: main
Are you sure you want to change the base?
Conversation
I'm also updating the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment, I'm mostly trying to reconcile these changes with our conversation earlier today. This is a sketch of my understanding from our discussion:
- Vanilla plugin: lives in tree, and it's a generic JS environment. This is not our current
core
crate. - When specifying
-C dynamic
and/or-C plugin
,-J
options are not allowed (this is the most simple route). We expect, at least initially or until someone actually needs this functionality that the plugin'sinitialize_runtime
will take care of initializing the JS runtime according to the plugin's needs. So no-J
options are needed immediately. -J
could be allowed for the vanilla plugin, which we don't have today and don't have to introduce immediately.- Ultimately our goal, at least in my head, is to remove the
core
crate entirely from Javy itself and treat it as plugin moving forward.
It's totally possible that I misunderstood some of our discussion points, so thanks for bearing with me.
In general, if my sketch above is not totally off, I think we can keep most of the changes here, except for the shared config parsing, which would involve eliminating parsing the -J
group from the CLI.
let mut config_bytes = vec![]; | ||
let shared_config = match io::stdin().read_to_end(&mut config_bytes) { | ||
Ok(0) => None, | ||
Ok(_) => Some(SharedConfig::parse_from_json(&config_bytes).unwrap()), | ||
Err(e) => panic!("Error reading from stdin: {e}"), | ||
}; | ||
let runtime = runtime::new(js_runtime_config).unwrap(); | ||
if let Some(shared_config) = shared_config { | ||
shared_config.apply_to_config(&mut config); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments are geared toward trying to reconcile the changes with our discussion earlier today; I think we agreed on going the simple route of not allowing any configuration to be passed for plugins from the CLI, except for the vanilla plugin. However, maybe we could've discussed what the vanilla plugin is; in my head it's not our current core
crate; in my mind the ideal state would be to remove the core
crate as we know it today from our tree and treat it as a plugin moving forward. I was thinking of vanilla as a generic JS environment, which we don't have today, and probably don't need to introduce immediately.
With that in mind:
- Can we instead remove the configuration overall? It's going to be a breaking change for sure, however some of the changes introduced for plugins already involve a breaking change.
- Aside from the breaking change aspect, I think the only thing that relies on the CLI flags affecting the runtime configuration are the integration tests.
Description of the change
This updates a few things relating to moving away from using a shared bitflag representation for JS configuration options between the Javy CLI,
javy-core
, and the Javy library crate:shared_config
Cargo feature is added to the Javy library crate. Enabling this feature exports a Wasm function calledconfig_schema
that outputs a JSON representation of all of the configuration properties the provider supports. It also exports some Rust data structures and associated methods for use by thejavy-core
crate. These data structures and methods are used to parse a JSON data structure representing a JS configuration sent by the Javy CLI and apply the parsed representation to a pre-existingJavy::Config
config.javy-core
has been updated to enable the newshared_config
feature and use its data structures to read in the JS configuration from the Javy CLI.config_schema
Wasm function exported from the provider and to validate the-J
flags against this schema. It has also been updated to encode the JS configuration as a JSON byte array when passing the JS configuration to theinitialize_runtime
function in the provider.javy-config
crate has been deleted which involved updating the Makefile and GitHub Action workflows.This change would not support runtime configurations that are not defined in the Javy crate. This change also only supports boolean configurations. It should be possible to augment the approach we're using if there is a need to in the future.
Why am I making this change?
Part of #768. Since plugins will exist outside of the Javy source tree, they cannot depend on the unpublished
javy-config
crate. Also it's easier for bitflags to get out of sync betweeen different source trees and when the bitflags get out of sync, the error messages that result are not helpful. So, we need a way for the Javy CLI to know what configuration properties a plugin supports along with associated help text. We also need a way for the CLI and plugin to understand each other when a configuration is passed intoinitialize_runtime
. JSON with using UTF-8 strings to describe the configuration properties seems like a relatively straightforward approach.Checklist
javy-cli
andjavy-core
do not require updating CHANGELOG files.