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

Don't attempt wasm-bindgen install if it exists in path. #81

Closed
wants to merge 3 commits into from

Conversation

data-pup
Copy link
Member

@data-pup data-pup commented Apr 13, 2018

(Fixing issue #51.)

After running into some of the problems in the previous PR here, I squashed the commits from my previous PR down, and rebased it off of the recent changes to the repo.

In case there are issues with the Travis build again, here are the rust-fmt and rustc versions I am using.

✨  cargo fmt --version
rustfmt 0.4.1-nightly (e784712 2018-04-09)

✨  rustc --version
rustc 1.27.0-nightly (ad610bed8 2018-04-11)

Thank you for bearing with me on the linting problems by the way, pardon me.

@mgattozzi
Copy link
Contributor

I'd just like to test this out locally but codewise I have no complaints! so it LGTM.

@ashleygwilliams
Copy link
Member

the conflicts here are cuz i ran committed and merged more cargo fmt stuff, lemme know if you want help fixing that :) and i'll check this out tomorrow! thanks so much!

@data-pup
Copy link
Member Author

data-pup commented Apr 14, 2018

Took a swing at fixing the merge conflict, the AppVeyor build failed with this message:

Downloading rustup-init.exe (4,701,696 bytes)...1%Error downloading remote file: One or more errors occurred.

That seems a little strange to me, not sure where to go from there.

@mgattozzi
Copy link
Contributor

Build probably needs to be restarted. Their service probably had an error connecting to it is all!

@data-pup
Copy link
Member Author

Added a note in the text to reference the issue being solved, per the pull request template checklist in pull #82. :)

@mgattozzi
Copy link
Contributor

Hey @data-pup there's going to be a bit of churn today with getting 0.2 release out but would you be able to do a rebase in the next few days so we can work on getting your pr in? :D

@data-pup
Copy link
Member Author

@mgattozzi Can do! I can get that done this afternoon. Congrats on 0.2 btw 🎉🙂

@mgattozzi
Copy link
Contributor

@data-pup awesome! Maybe wait till we get the changelog pr in so you're not having to rebase again. There's no rush. Just wanted to make sure we're keeping an eye on this PR :)

@data-pup
Copy link
Member Author

Sounds good! Feel free to ping me once things are in a good position for me to rebase from :)

@data-pup
Copy link
Member Author

data-pup commented Apr 25, 2018

I went ahead and rebased this. I can do that again once #106 lands, but this hopefully gets all of the merging conflicts out of the way. 😀

I can also add some error handling to the wasm_bindgen_installed function and have it return a Result<(), Error> like you added in elsewhere. We mentioned that potentially being out of scope, but I wouldn't mind taking care of that too!

(Update: Took a swing at adding error handling, let me know if there are any changes there to make.)

@mgattozzi
Copy link
Contributor

Since I put in a refactor for the error handling it's actually probably best that you put it in :)

@data-pup
Copy link
Member Author

Rad :D If the error handling in there needs any changes, I can take another pass at it

@mgattozzi
Copy link
Contributor

I think it's fine for now. Later on we'll be doing better error messages. I'll take a look at this on my Windows computer tonight :D

@data-pup
Copy link
Member Author

data-pup commented May 2, 2018

Did #120 end up taking care of this? I can resolve the conflicts here if not :)

@ashleygwilliams
Copy link
Member

heh yeah i was in the process of commenting on this @data-pup - i think the open question re https://github.com/ashleygwilliams/wasm-pack/issues/115 means this is a bit still on hold - the error PR from @mgattozzi shouldn't have affected this necessarily- the key question here is still how to manage installing wasm-bindgen. as that is still an open question, i think this PR is blocked for now, but i also think that this PR is getting us closer to an intermediate solution, so let's leave it open and see how 115 plays out.

@data-pup
Copy link
Member Author

data-pup commented May 2, 2018

Sounds good :) I’ll keep an eye on that, let me know if anything changes!

@ashleygwilliams
Copy link
Member

so i opened up a new issue about how we should address this- #146 i think we can use a lot of the work that's here as the basis for this. i'm already frustrated that we install wasm-bindgen everytime now lol. let me know if you have any thoughts or questsions, @data-pup !

@ashleygwilliams ashleygwilliams added this to the 0.4.0 milestone Jun 5, 2018
@data-pup
Copy link
Member Author

data-pup commented Jun 5, 2018

Awesome! That sounds good to me, when Alex confirms that the strategy mentioned over there would work I can wrap this up 😄 Thanks for following up on this @ashleygwilliams!

@ashleygwilliams ashleygwilliams modified the milestones: 0.4.0, 0.5.0 Jun 18, 2018
@data-pup data-pup mentioned this pull request Jun 19, 2018
3 tasks
@data-pup data-pup closed this Jun 19, 2018
@data-pup data-pup deleted the add-install-check branch October 13, 2018 18:19
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.

3 participants