Skip to content
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

Implements RFC 8, fixing #606 #986

Merged
merged 1 commit into from
Oct 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/command/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ fn find_manifest_from_cwd() -> Result<PathBuf, failure::Error> {

/// Construct our `pkg` directory in the crate.
pub fn create_pkg_dir(out_dir: &Path) -> Result<(), failure::Error> {
let _ = fs::remove_dir_all(&out_dir); // Clean up any existing directory and ignore errors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. It's unclear if we need to remove the entire out directory. It may not be a big deal but say someone has some npm link going on pointed at the pkg directory, this may break it on every rebuild. I'm not sure if npm link does a symlink or a hard link (or is that not a thing with directories) which I think would be the difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can see your point. npm link documentation claims they are symlinks.

That said, I don't feel strongly about this. I personally tend to want my tests to run on a as clean as possible environment to avoid spurious failures due to other tests leaving cruft behind, but I'm happy to remove this cleanup if that makes sense to you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're seeing errors that I think are in relation to this as reported at #1099 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've raised a PR to revert this change: #1110

fs::create_dir_all(&out_dir)?;
fs::write(out_dir.join(".gitignore"), "*")?;
Ok(())
Expand Down
43 changes: 36 additions & 7 deletions src/manifest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@

mod npm;

use std::fs;
use std::path::Path;
use std::{collections::HashMap, fs};

use self::npm::{
repository::Repository, CommonJSPackage, ESModulesPackage, NoModulesPackage, NpmPackage,
Expand Down Expand Up @@ -575,14 +575,25 @@ impl CrateData {
target: Target,
) -> Result<(), Error> {
let pkg_file_path = out_dir.join("package.json");
// Check if a `package.json` was already generated by wasm-bindgen, if so
// we merge the NPM dependencies already specified in it.
let existing_deps = if pkg_file_path.exists() {
// It's just a map of dependency names to versions
Some(serde_json::from_str::<HashMap<String, String>>(
&fs::read_to_string(&pkg_file_path)?,
)?)
} else {
None
};
let npm_data = match target {
Target::Nodejs => self.to_commonjs(scope, disable_dts, out_dir),
Target::NoModules => self.to_nomodules(scope, disable_dts, out_dir),
Target::Bundler => self.to_esmodules(scope, disable_dts, out_dir),
Target::Web => self.to_web(scope, disable_dts, out_dir),
Target::Nodejs => self.to_commonjs(scope, disable_dts, existing_deps, out_dir),
Target::NoModules => self.to_nomodules(scope, disable_dts, existing_deps, out_dir),
Target::Bundler => self.to_esmodules(scope, disable_dts, existing_deps, out_dir),
Target::Web => self.to_web(scope, disable_dts, existing_deps, out_dir),
};

let npm_json = serde_json::to_string_pretty(&npm_data)?;

fs::write(&pkg_file_path, npm_json)
.with_context(|_| format!("failed to write: {}", pkg_file_path.display()))?;
Ok(())
Expand Down Expand Up @@ -657,7 +668,13 @@ impl CrateData {
})
}

fn to_commonjs(&self, scope: &Option<String>, disable_dts: bool, out_dir: &Path) -> NpmPackage {
fn to_commonjs(
&self,
scope: &Option<String>,
disable_dts: bool,
dependencies: Option<HashMap<String, String>>,
out_dir: &Path,
) -> NpmPackage {
let data = self.npm_data(scope, false, disable_dts, out_dir);
let pkg = &self.data.packages[self.current_idx];

Expand All @@ -683,13 +700,15 @@ impl CrateData {
homepage: data.homepage,
types: data.dts_file,
keywords: data.keywords,
dependencies,
})
}

fn to_esmodules(
&self,
scope: &Option<String>,
disable_dts: bool,
dependencies: Option<HashMap<String, String>>,
out_dir: &Path,
) -> NpmPackage {
let data = self.npm_data(scope, true, disable_dts, out_dir);
Expand Down Expand Up @@ -718,10 +737,17 @@ impl CrateData {
types: data.dts_file,
side_effects: false,
keywords: data.keywords,
dependencies,
})
}

fn to_web(&self, scope: &Option<String>, disable_dts: bool, out_dir: &Path) -> NpmPackage {
fn to_web(
&self,
scope: &Option<String>,
disable_dts: bool,
dependencies: Option<HashMap<String, String>>,
out_dir: &Path,
) -> NpmPackage {
let data = self.npm_data(scope, false, disable_dts, out_dir);
let pkg = &self.data.packages[self.current_idx];

Expand All @@ -748,13 +774,15 @@ impl CrateData {
types: data.dts_file,
side_effects: false,
keywords: data.keywords,
dependencies,
})
}

fn to_nomodules(
&self,
scope: &Option<String>,
disable_dts: bool,
dependencies: Option<HashMap<String, String>>,
out_dir: &Path,
) -> NpmPackage {
let data = self.npm_data(scope, false, disable_dts, out_dir);
Expand Down Expand Up @@ -782,6 +810,7 @@ impl CrateData {
homepage: data.homepage,
types: data.dts_file,
keywords: data.keywords,
dependencies,
})
}

Expand Down
4 changes: 4 additions & 0 deletions src/manifest/npm/commonjs.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::collections::HashMap;

use manifest::npm::repository::Repository;

#[derive(Serialize)]
Expand All @@ -21,4 +23,6 @@ pub struct CommonJSPackage {
pub types: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub keywords: Option<Vec<String>>,
#[serde(skip_serializing_if = "Option::is_none")]
pub dependencies: Option<HashMap<String, String>>,
}
4 changes: 4 additions & 0 deletions src/manifest/npm/esmodules.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::collections::HashMap;

use manifest::npm::repository::Repository;

#[derive(Serialize)]
Expand All @@ -23,4 +25,6 @@ pub struct ESModulesPackage {
pub side_effects: bool,
#[serde(skip_serializing_if = "Option::is_none")]
pub keywords: Option<Vec<String>>,
#[serde(skip_serializing_if = "Option::is_none")]
pub dependencies: Option<HashMap<String, String>>,
}
4 changes: 4 additions & 0 deletions src/manifest/npm/nomodules.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::collections::HashMap;

use manifest::npm::repository::Repository;

#[derive(Serialize)]
Expand All @@ -21,4 +23,6 @@ pub struct NoModulesPackage {
pub types: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub keywords: Option<Vec<String>>,
#[serde(skip_serializing_if = "Option::is_none")]
pub dependencies: Option<HashMap<String, String>>,
}
50 changes: 49 additions & 1 deletion tests/all/manifest.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use assert_cmd::prelude::*;
use std::collections::HashSet;
use std::collections::{HashMap, HashSet};
use std::fs;
use std::path::PathBuf;
use utils::{self, fixture};
Expand Down Expand Up @@ -314,6 +314,54 @@ fn it_creates_a_package_json_with_correct_keys_when_types_are_skipped() {
assert_eq!(actual_files, expected_files);
}

#[test]
fn it_creates_a_package_json_with_npm_dependencies_provided_by_wasm_bindgen() {
let fixture = fixture::js_hello_world();
let out_dir = fixture.path.join("pkg");
let crate_data = manifest::CrateData::new(&fixture.path, None).unwrap();
wasm_pack::command::utils::create_pkg_dir(&out_dir).unwrap();
// Write a `package.json` in the out_dir, as wasm-bindgen does:
utils::manifest::create_wbg_package_json(
&out_dir,
r#"
{ "foo": "^1.2.3" }
"#,
)
.unwrap();
assert!(crate_data
.write_package_json(&out_dir, &None, true, Target::Bundler)
.is_ok());
let package_json_path = &out_dir.join("package.json");
fs::metadata(package_json_path).unwrap();
utils::manifest::read_package_json(&fixture.path, &out_dir).unwrap();
let pkg = utils::manifest::read_package_json(&fixture.path, &out_dir).unwrap();
assert_eq!(pkg.name, "js-hello-world");
assert_eq!(pkg.repository.ty, "git");
assert_eq!(
pkg.repository.url,
"https://github.com/rustwasm/wasm-pack.git"
);
assert_eq!(pkg.module, "js_hello_world.js");

let actual_files: HashSet<String> = pkg.files.into_iter().collect();
let expected_files: HashSet<String> = [
"js_hello_world_bg.wasm",
"js_hello_world_bg.js",
"js_hello_world.js",
]
.iter()
.map(|&s| String::from(s))
.collect();
assert_eq!(actual_files, expected_files);

let dependencies: Option<HashMap<String, String>> = pkg.dependencies;
assert!(dependencies.is_some());
let mut expected_dependencies: HashMap<String, String> = HashMap::new();
expected_dependencies.insert("foo".to_owned(), "^1.2.3".to_owned());

assert_eq!(dependencies.unwrap(), expected_dependencies);
}

#[test]
fn it_errors_when_wasm_bindgen_is_not_declared() {
let fixture = fixture::bad_cargo_toml();
Expand Down
8 changes: 7 additions & 1 deletion tests/all/utils/manifest.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::fs::File;
use std::io::prelude::*;
use std::path::Path;
use std::{collections::HashMap, fs::File};

use failure::Error;
use serde_json;
Expand All @@ -25,6 +25,7 @@ pub struct NpmPackage {
pub side_effects: bool,
pub homepage: Option<String>,
pub keywords: Option<Vec<String>>,
pub dependencies: Option<HashMap<String, String>>,
}

fn default_none() -> String {
Expand All @@ -50,3 +51,8 @@ pub fn read_package_json(path: &Path, out_dir: &Path) -> Result<NpmPackage, Erro

Ok(serde_json::from_str(&pkg_contents)?)
}

pub fn create_wbg_package_json(out_dir: &Path, contents: &str) -> Result<(), Error> {
let manifest_path = out_dir.join("package.json");
Ok(std::fs::write(manifest_path, contents)?)
}