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.

Sign up for GitHub

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

Jump to bottom

Corrected files included in package.json for bundler / no target (#837) #839

Merged
merged 5 commits into from
May 5, 2020
Merged

Corrected files included in package.json for bundler / no target (#837) #839

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bad4282
Working but may need changes
lucashorward May 4, 2020
5039905
Improved setup
lucashorward May 4, 2020
af9b8ac
Format
lucashorward May 4, 2020
a22ce34
Merge branch 'master' into build-files
lucashorward May 5, 2020
06ba980
Made changes according to comment
lucashorward May 5, 2020
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
8 changes: 4 additions & 4 deletions src/manifest/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ impl CrateData {
fn npm_data(
&self,
scope: &Option<String>,
include_commonjs_shim: bool,
add_js_bg_to_package_json: bool,
disable_dts: bool,
out_dir: &Path,
) -> NpmData {
Expand All @@ -588,7 +588,7 @@ impl CrateData {
let mut files = vec![wasm_file];

files.push(js_file.clone());
if include_commonjs_shim {
if add_js_bg_to_package_json {
let js_bg_file = format!("{}_bg.js", name_prefix);
files.push(js_bg_file);
}
Expand Down Expand Up @@ -671,7 +671,7 @@ impl CrateData {
disable_dts: bool,
out_dir: &Path,
) -> NpmPackage {
let data = self.npm_data(scope, false, disable_dts, out_dir);
let data = self.npm_data(scope, true, disable_dts, out_dir);
let pkg = &self.data.packages[self.current_idx];

self.check_optional_fields();
Expand Down Expand Up @@ -734,7 +734,7 @@ impl CrateData {
disable_dts: bool,
out_dir: &Path,
) -> NpmPackage {
let data = self.npm_data(scope, false, disable_dts, out_dir);
let data = self.npm_data(scope, true, disable_dts, out_dir);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be false. The to_commonjs method should also use false, only to_esmodules should be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then there would be no change in behavior, and the issue as described would not be fixed? Please let me know if there is a better way of fixing this issue!

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the situation in the old code:

  • to_commonjs -> true
  • to_esmodules -> false
  • to_web -> false
  • to_nomodules -> false

This is the situation in your code:

  • to_commonjs -> true
  • to_esmodules -> true
  • to_web -> false
  • to_nomodules -> true

However it is supposed to be like this:

  • to_commonjs -> false
  • to_esmodules -> true
  • to_web -> false
  • to_nomodules -> false

It is only the bundler target (to_esmodules) which has the extra _bg.js file, the rest don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the explanation - I have made the changes. However, I noticed in this and my previous pr that the test build_with_and_without_wasm_bindgen_debug keeps failing locally, and now I see that the same has happened on master. Originally I also had this issue on master locally so I thought it was a configuration issue on my side, but as it also fails in the CI I doubt that that is the case. Do you have any idea why this test fails?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I need to look into why that's failing, you can ignore that.

let pkg = &self.data.packages[self.current_idx];

self.check_optional_fields();
Expand Down
33 changes: 21 additions & 12 deletions tests/all/manifest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,9 @@ fn it_creates_a_package_json_default_path() {

let actual_files: HashSet<String> = pkg.files.into_iter().collect();
let expected_files: HashSet<String> = [
"js_hello_world_bg.wasm",
"js_hello_world.d.ts",
"js_hello_world_bg.js",
"js_hello_world_bg.wasm",
"js_hello_world.js",
]
.iter()
Expand All @@ -125,8 +126,9 @@ fn it_creates_a_package_json_provided_path() {

let actual_files: HashSet<String> = pkg.files.into_iter().collect();
let expected_files: HashSet<String> = [
"js_hello_world_bg.wasm",
"js_hello_world.d.ts",
"js_hello_world_bg.js",
"js_hello_world_bg.wasm",
"js_hello_world.js",
]
.iter()
Expand All @@ -153,8 +155,9 @@ fn it_creates_a_package_json_provided_path_with_scope() {

let actual_files: HashSet<String> = pkg.files.into_iter().collect();
let expected_files: HashSet<String> = [
"js_hello_world_bg.wasm",
"js_hello_world.d.ts",
"js_hello_world_bg.js",
"js_hello_world_bg.wasm",
"js_hello_world.js",
]
.iter()
Expand Down Expand Up @@ -222,9 +225,10 @@ fn it_creates_a_pkg_json_with_correct_files_on_nomodules() {

let actual_files: HashSet<String> = pkg.files.into_iter().collect();
let expected_files: HashSet<String> = [
"js_hello_world.d.ts",
"js_hello_world_bg.js",
"js_hello_world_bg.wasm",
"js_hello_world.js",
"js_hello_world.d.ts",
]
.iter()
.map(|&s| String::from(s))
Expand Down Expand Up @@ -256,10 +260,11 @@ fn it_creates_a_package_json_with_correct_files_when_out_name_is_provided() {
assert_eq!(pkg.side_effects, false);

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

Expand Down Expand Up @@ -300,10 +305,14 @@ fn it_creates_a_package_json_with_correct_keys_when_types_are_skipped() {
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.js"]
.iter()
.map(|&s| String::from(s))
.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);
}

Expand Down