-
Notifications
You must be signed in to change notification settings - Fork 409
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 --skip-build flag for init command #144 #151
Add --skip-build flag for init command #144 #151
Conversation
5e7f4eb
to
0960881
Compare
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.
hey! i've given this a first pass and tbh i think it's really fantastic- awesome job! i have a few comments, but one is a style nit and the other is potentially something to implement at a later date. i want to pull this down and run it (not cuz i don't trust you, but because i don't trust our tests)... but in general i think this is gonna be good to merge very soon!
src/command.rs
Outdated
target, | ||
crate_name: None, | ||
} | ||
} |
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.
would like a line here :)
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.
oups,
yes sure
src/command.rs
Outdated
} | ||
pub fn process(&mut self, log: &Logger) -> result::Result<(), Error> { | ||
let steps: Vec<fn(&mut Init, &Step, &Logger) -> result::Result<(), Error>> = | ||
if self.skip_build { |
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.
(ugh GH didn't post this comment in my original review)
so- i think we might be exploring even more "build" modes (see #153 ) - so (and i am likely overengineering here) but perhaps an enum and a match statement would work best here. would love to hear your thoughts!
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.
Do you mean something like this ?
enum InitMode {
Normal,
Nobuild,
}
impl Init {
pub fn process(&mut self, log: &Logger, mode: InitMode) -> result::Result<(), Error> {
let steps: Vec<fn(&mut Init, &Step, &Logger) -> result::Result<(), Error>> =
match mode {
InitMode::Normal => vec![
Init::step_add_wasm_target,
...
],
InitMode::Nobuild => vec![
Init::step_create_dir,
...
],
};
let mut step = Step::new(steps.len());
...
}
}
pub fn run_wasm_pack(command: Command, log: &Logger) -> result::Result<(), Error> {
let status = match command {
Command::Init {
path,
scope,
skip_build,
disable_dts,
target,
} => {
...
let mode = if skip_build {InitMode::Nobuild} else {InitMode::Normal};
Init::new(path, scope, disable_dts, target).process(&log, mode)
}
...
};
...
}
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.
I did the modification this modification in the PR, if it is not like you what you had in mind I will revert it.
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.
I never thought to make it a vector of functions. Honestly this is just a real clever PR and I really like it. My own nit is I don't think we need to rename every function to step_* but that's more of a personal preference and I'll defer to @ashleygwilliams on this! Seriously well done. Gonna have to test this out locally as well! :D
@kohensu super impressed with this- thanks so much! the only thing this needs is a test and some docs (a paragraph like the other flags in this doc: https://github.com/ashleygwilliams/wasm-pack/blob/master/docs/init.md). lemme know if you wanna do that or if you'd like some help! |
also- just as a heads up we are probably landing 2 PRs today which means this will likely need a rebase, but i'd like to prioritize getting this in after that so that we don't have too much rebasing that needs to happen :) |
lemme know if you want to tackle the rebase- otherwise i can do it! |
ok, I will do the rebase, the test and the doc |
1576fc1
to
c3cc16b
Compare
This flag skips: adding target compiling to wasm installing wasm-bindgen running wasm-bindgen
bfecca7
to
74fe847
Compare
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.
Just wanted to say that this LGTM :) great job!
pull request for #144