-
Notifications
You must be signed in to change notification settings - Fork 115
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 --no-source-compression option to reduce compile time #581
Conversation
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.
Following the discussion in #579, I think that adding a way to control compression level from the CLI is a good addition. I'm struggling a bit with approach of using the --dev
flag and its implications. This flag (to me at least) signals that at a group of settings will be modified to enhance development mode. In this case, I can think of at least one more setting that could be affected by this flag: the code generation optimization options. Currently these settings are hardcoded, but one could argue that when the --dev
flag is present, we shouldn't perform any optimizations at all.
I don't want to block this PR from landing, but if the compression level is the only setting that you're currently interested in changing for your use-case, I'd suggest switching to a less generic flag.
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.
Thanks for the contribution! Sorry for the back and forth on some of the details but I'm optimistic we should be able to get some form of this landed!
crates/cli/src/commands.rs
Outdated
@@ -34,6 +34,10 @@ pub struct CompileCommandOpts { | |||
#[structopt(short = "n")] | |||
/// Optional WIT world name for WIT file. Must be specified if WIT is file path is specified. | |||
pub wit_world: Option<String>, | |||
|
|||
#[structopt(long = "dev")] |
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.
Given @saulecabrera's feedback, let's switch this to --no-source-compression
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.
fixed!
|
||
pub fn uncompressed(js: &JS) -> Result<SourceCodeSection> { | ||
Ok(SourceCodeSection { | ||
compressed_source_code: js.as_bytes().to_vec(), |
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.
We should rename this field if it may contain compressed or uncompressed source code. I think we can just call it source_code
.
Also I'd prefer if we changed the name of ::new
to ::compressed
so it's more obvious what the difference with uncompressed
is.
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.
fixed!
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.
Thank you for your contribution!
I'll get started on creating a release for this.
@jeffcharles |
close #579
Description of the change
Add --no-source-compression option to reduce compile time
Why am I making this change?
ref: #579
Checklist
javy-cli
andjavy-core
do not require updating CHANGELOG files.