Repository: ashleygwilliams/wasm-pack
Pull request #109 (2018-04-25 19:59:45)

feat(command): expose --no-typescript option to init command
- relates to #107 

I got so excited about #107 as typescript user so gave a quick stab, but not sure if this change is intended scope for issue itself. Feel freely close / or suggest as needed. 🙏 

This PR intends to ask bindgen to generate typescript definition as well, also expand `NpmPackage` struct to include `types` property into `package.json`. It currently takes naïve approach by specifying type definition to same entrypoint of `main`.

Make sure these boxes are checked! 📦✅

- [ ] You have the latest version of `rustfmt` installed and have your 
      cloned directory set to nightly
```bash
$ rustup override set nightly
$ rustup component add rustfmt-preview --toolchain nightly
```
- [x] You ran `rustfmt` on the code base before submitting
- [x] You reference which issue is being closed in the PR text

✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨


mgattozzi: Hey @kwonoj thanks for the PR! So this is definitely a good start but there are somethings in #107 that weren't exactly mentioned which is this should be optional! The invocation would be along the lines of `wasm-pack init --typescript` meaning that instead of the default behavior it adds the typescript flag to wasm-bindgen. The way you have it now typescript is the default.

Your enthusiasm is great so if you want to tackle 107 completely let's get this PR into shape before merging! Do you need help in understanding what needs to be changed in order to get what I described above? I'm more than happy to type those instructions out for you :D
kwonoj: @mgattozzi Thanks for guidance.

For matter of 

>instead of the default behavior it adds the typescript flag to wasm-bindgen. The way you have it now typescript is the default.

Is it necessarily have to be opt-in? in case of consumer point of npm packages, any JS user doesn't get interrupted by existence of type definition, while TS users will definitely want to have types most times. In which cases it'd be encouraged to generate npm packages doesn't include type definitions?
kwonoj: (Clarifying, I can definitely change PR to enable opt in behavior - just wanted to understand usecases before getting into)
mgattozzi: From my conversations with @ashleygwilliams my understanding is opt-in, but I didn't know about this behavior before. If none of that matters then maybe it can be implicitly put in but that's not my call.
kwonoj: I see. Maybe it depends on how accurate generated type definition matches to original signature in rust code - but if assumption's it is correct enough, I think it'd better to have types by default. (Or can make opt-out to not to have it). Typescript definition's primary consumer is obviously typescript users, but also lot of tooling like vs code's lsp can understand those so plain js user gets additional benefits such as intellisense. Course actual js build pipeline doesn't affected by existence of type definition.

Still, lett me know if desired behavior should be opt in - I'll update PR accordingly.
Pauan: It's not my decision to make either, but I would just like to point out that it's common (and recommended) to compile TypeScript projects to `.js` + `.d.ts`, so that way they can be used seamlessly with either ES6 or TypeScript. In particular, it is *not* recommended to publish `.ts` files.
kwonoj: @Pauan definitely, and discusdions about whthere include d.ts or not. Wasm-bindgen's flag generates 'd.ts' as type definition and doesn't generate typescript files, reason I thought it can be default.
robertohuertasm: I agree with @kwonoj. I think `.d.ts` files should be created by default. As he states it only adds value and doesn't interfere with users consuming it from a plain js project; on the contrary, it will help their IDE to provide full intellisense (like in VSCode for instance).
ashleygwilliams: hi! i definitely do not think that typescript should *NOT* be the default. i really like having the option, and am open to promoting it to default at some point if that seems appropriate, but for landing this i would like to see it behind a `--typescript` flag! i'm certainly open to changing my mind - but let's start small, land this behind a flag, and then continue this convo in a new issue. i'll open one :) thanks everyone for your thoughtful convo! i'm excited to chat more about this.
mgattozzi: Sweet. @kwonoj let me know if you need help at all to do this! :D
kwonoj: Ok, sounds fair - I'll try to update PR to have flags.
ashleygwilliams: @kwonoj actually.... i think u might have convinced me .... https://github.com/ashleygwilliams/wasm-pack/issues/114#issuecomment-384856738 lemme think on this, but i think i might be down to have this land sans-flag, you make some very compelling points

Patch:
diff --git a/src/bindgen.rs b/src/bindgen.rs
index a640c0e..c85b7e3 100644
--- a/src/bindgen.rs
+++ b/src/bindgen.rs
@@ -29,7 +29,7 @@ pub fn cargo_install_wasm_bindgen() -> Result<(), Error> {
     }
 }
 
-pub fn wasm_bindgen_build(path: &str, name: &str) -> Result<(), Error> {
+pub fn wasm_bindgen_build(path: &str, name: &str, disable_dts: bool) -> Result<(), Error> {
     let step = format!(
         "{} {}Running WASM-bindgen...",
         style("[7/7]").bold().dim(),
@@ -38,11 +38,18 @@ pub fn wasm_bindgen_build(path: &str, name: &str) -> Result<(), Error> {
     let pb = PBAR.message(&step);
     let binary_name = name.replace("-", "_");
     let wasm_path = format!("target/wasm32-unknown-unknown/release/{}.wasm", binary_name);
+    let dts_arg = if disable_dts == false {
+        "--typescript"
+    } else {
+        "--no-typescript"
+    };
+
     let output = Command::new("wasm-bindgen")
         .current_dir(path)
         .arg(&wasm_path)
         .arg("--out-dir")
         .arg("./pkg")
+        .arg(dts_arg)
         .output()?;
     pb.finish();
     if !output.status.success() {
diff --git a/src/command.rs b/src/command.rs
index bd80321..b8ee0bd 100644
--- a/src/command.rs
+++ b/src/command.rs
@@ -22,6 +22,8 @@ pub enum Command {
         path: Option<String>,
         #[structopt(long = "scope", short = "s")]
         scope: Option<String>,
+        #[structopt(long = "no-typescript")]
+        disable_dts: bool,
     },
     #[structopt(name = "pack")]
     /// 🍱  create a tar of your npm package but don't publish! [NOT IMPLEMENTED]
@@ -35,7 +37,11 @@ pub fn run_wasm_pack(command: Command) -> result::Result<(), Error> {
     // Run the correct command based off input and store the result of it so that we can clear
     // the progress bar then return it
     let status = match command {
-        Command::Init { path, scope } => init(path, scope),
+        Command::Init {
+            path,
+            scope,
+            disable_dts,
+        } => init(path, scope, disable_dts),
         Command::Pack { path } => pack(path),
         Command::Publish { path } => publish(path),
     };
@@ -65,7 +71,11 @@ pub fn create_pkg_dir(path: &str) -> result::Result<(), Error> {
     Ok(())
 }
 
-fn init(path: Option<String>, scope: Option<String>) -> result::Result<(), Error> {
+fn init(
+    path: Option<String>,
+    scope: Option<String>,
+    disable_dts: bool,
+) -> result::Result<(), Error> {
     let started = Instant::now();
 
     let crate_path = set_crate_path(path);
@@ -73,11 +83,11 @@ fn init(path: Option<String>, scope: Option<String>) -> result::Result<(), Error
     build::rustup_add_wasm_target()?;
     build::cargo_build_wasm(&crate_path)?;
     create_pkg_dir(&crate_path)?;
-    manifest::write_package_json(&crate_path, scope)?;
+    manifest::write_package_json(&crate_path, scope, disable_dts)?;
     readme::copy_from_crate(&crate_path)?;
     bindgen::cargo_install_wasm_bindgen()?;
     let name = manifest::get_crate_name(&crate_path)?;
-    bindgen::wasm_bindgen_build(&crate_path, &name)?;
+    bindgen::wasm_bindgen_build(&crate_path, &name, disable_dts)?;
     PBAR.message(&format!(
         "{} Done in {}",
         emoji::SPARKLE,
diff --git a/src/manifest.rs b/src/manifest.rs
index 9ea770f..4447912 100644
--- a/src/manifest.rs
+++ b/src/manifest.rs
@@ -33,6 +33,7 @@ struct NpmPackage {
     repository: Option<Repository>,
     files: Vec<String>,
     main: String,
+    types: Option<String>,
 }
 
 #[derive(Serialize)]
@@ -52,10 +53,16 @@ fn read_cargo_toml(path: &str) -> Result<CargoManifest, Error> {
 }
 
 impl CargoManifest {
-    fn into_npm(mut self, scope: Option<String>) -> NpmPackage {
+    fn into_npm(mut self, scope: Option<String>, disable_dts: bool) -> NpmPackage {
         let filename = self.package.name.replace("-", "_");
         let wasm_file = format!("{}_bg.wasm", filename);
         let js_file = format!("{}.js", filename);
+        let dts_file = if disable_dts == true {
+            None
+        } else {
+            Some(format!("{}.d.ts", filename))
+        };
+
         if let Some(s) = scope {
             self.package.name = format!("@{}/{}", s, self.package.name);
         }
@@ -71,12 +78,17 @@ impl CargoManifest {
             }),
             files: vec![wasm_file],
             main: js_file,
+            types: dts_file,
         }
     }
 }
 
 /// Generate a package.json file inside in `./pkg`.
-pub fn write_package_json(path: &str, scope: Option<String>) -> Result<(), Error> {
+pub fn write_package_json(
+    path: &str,
+    scope: Option<String>,
+    disable_dts: bool,
+) -> Result<(), Error> {
     let step = format!(
         "{} {}Writing a package.json...",
         style("[4/7]").bold().dim(),
@@ -94,7 +106,7 @@ pub fn write_package_json(path: &str, scope: Option<String>) -> Result<(), Error
     let pkg_file_path = format!("{}/pkg/package.json", path);
     let mut pkg_file = File::create(pkg_file_path)?;
     let crate_data = read_cargo_toml(path)?;
-    let npm_data = crate_data.into_npm(scope);
+    let npm_data = crate_data.into_npm(scope, disable_dts);
 
     if npm_data.description.is_none() {
         PBAR.warn(&warn_fmt("description"));


Test patch:
diff --git a/tests/manifest/main.rs b/tests/manifest/main.rs
index 813087d..6510374 100644
--- a/tests/manifest/main.rs
+++ b/tests/manifest/main.rs
@@ -29,7 +29,7 @@ fn it_gets_the_crate_name_provided_path() {
 fn it_creates_a_package_json_default_path() {
     let path = ".".to_string();
     wasm_pack::command::create_pkg_dir(&path).unwrap();
-    assert!(manifest::write_package_json(&path, None).is_ok());
+    assert!(manifest::write_package_json(&path, None, false).is_ok());
     let package_json_path = format!("{}/pkg/package.json", &path);
     assert!(fs::metadata(package_json_path).is_ok());
     assert!(utils::read_package_json(&path).is_ok());
@@ -42,13 +42,15 @@ fn it_creates_a_package_json_default_path() {
     );
     assert_eq!(pkg.files, ["wasm_pack_bg.wasm"]);
     assert_eq!(pkg.main, "wasm_pack.js");
+    let types = pkg.types.unwrap_or_default();
+    assert_eq!(types, "wasm_pack.d.ts");
 }
 
 #[test]
 fn it_creates_a_package_json_provided_path() {
     let path = "tests/fixtures/js-hello-world".to_string();
     wasm_pack::command::create_pkg_dir(&path).unwrap();
-    assert!(manifest::write_package_json(&path, None).is_ok());
+    assert!(manifest::write_package_json(&path, None, false).is_ok());
     let package_json_path = format!("{}/pkg/package.json", &path);
     assert!(fs::metadata(package_json_path).is_ok());
     assert!(utils::read_package_json(&path).is_ok());
@@ -60,7 +62,7 @@ fn it_creates_a_package_json_provided_path() {
 fn it_creates_a_package_json_provided_path_with_scope() {
     let path = "tests/fixtures/scopes".to_string();
     wasm_pack::command::create_pkg_dir(&path).unwrap();
-    assert!(manifest::write_package_json(&path, Some("test".to_string())).is_ok());
+    assert!(manifest::write_package_json(&path, Some("test".to_string()), false).is_ok());
     let package_json_path = format!("{}/pkg/package.json", &path);
     assert!(fs::metadata(package_json_path).is_ok());
     assert!(utils::read_package_json(&path).is_ok());
diff --git a/tests/manifest/utils.rs b/tests/manifest/utils.rs
index aa97d98..25add46 100644
--- a/tests/manifest/utils.rs
+++ b/tests/manifest/utils.rs
@@ -13,6 +13,7 @@ pub struct NpmPackage {
     pub repository: Repository,
     pub files: Vec<String>,
     pub main: String,
+    pub types: Option<String>,
 }
 
 #[derive(Deserialize)]