Skip to content

feat(manifest): add main entry #95

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

Merged
merged 7 commits into from
Apr 19, 2018
Merged

feat(manifest): add main entry #95

merged 7 commits into from
Apr 19, 2018

Conversation

ashleygwilliams
Copy link
Member

@ashleygwilliams ashleygwilliams commented Apr 19, 2018

fixes #94

this PR does a few things:

  • adds a step to the wasm_bindgen run that renames the generated js file to index.js
  • adds a main entry to the package.json that points to index.js
  • removes the js file from the files entry since it's preservation in the package is cover by being in the main entry
  • removes redundant/unnecessary checks on package.json contents in some tests

@ashleygwilliams ashleygwilliams removed the work in progress do not merge! label Apr 19, 2018
@ashleygwilliams ashleygwilliams changed the title [WIP] feat(manifest): add main entry feat(manifest): add main entry Apr 19, 2018
Copy link
Contributor

@mgattozzi mgattozzi left a comment

Choose a reason for hiding this comment

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

This looks great. I had a question but I think that's for another pr or issue. Namely how do we keep wasm -bindgen up to date. If there's nothing else you need to change let's get this pr merged! :D

@@ -13,11 +14,15 @@ pub fn cargo_install_wasm_bindgen() -> Result<(), Error> {
let pb = PBAR.message(&step);
let output = Command::new("cargo")
.arg("install")
.arg("wasm-bindgen")
.arg("wasm-bindgen-cli")
Copy link
Contributor

Choose a reason for hiding this comment

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

ah that was the error you mentioned before nice catch!

.output()?;
pb.finish();
if !output.status.success() {
let s = String::from_utf8_lossy(&output.stderr);
if s.contains("already exists") {
PBAR.one_off_message("wasm-bindgen already installed");
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there's a newer version that needs to be installed? I think we can add that in a separate PR though. This is just to patch broken behavior for now

@@ -46,6 +51,9 @@ pub fn wasm_bindgen_build(path: &str, name: &str) -> Result<(), Error> {
PBAR.error("wasm-bindgen failed to execute properly");
bail!(format!("Details:\n{}", s));
} else {
let js_file = format!("{}/pkg/{}.js", path, binary_name);
let index_file = format!("{}/pkg/index.js", path);
fs::rename(&js_file, &index_file)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay I see. We didn't have an index.js file so npm complained. This part looks good to me

Copy link
Member Author

Choose a reason for hiding this comment

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

npm didn't complain but this is leveraging npm better

@ashleygwilliams ashleygwilliams merged commit 40a5e66 into master Apr 19, 2018
@ashleygwilliams ashleygwilliams deleted the index-main branch April 19, 2018 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Declare a "main file" or index.js in the generated module
2 participants