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

should we -f wasm-bindgen install? #115

Closed
ashleygwilliams opened this issue Apr 27, 2018 · 13 comments · Fixed by #133
Closed

should we -f wasm-bindgen install? #115

ashleygwilliams opened this issue Apr 27, 2018 · 13 comments · Fixed by #133
Labels
PR attached there's a PR open for this issue to-do stuff that needs to happen, so plz do it k thx
Milestone

Comments

@ashleygwilliams
Copy link
Member

pros:

  • will fix most bug reports we've received thus far on this (the bug is that wasm-bindgen is a later version)

cons:

  • will install wasm-bindgen even if it's already installed, so it will take longer

my desired behavior here would be similar to using the npm install @latest tag, which checks if the current installed version is the same as the version tag latest, if not installs, if so does nothing. i don't think cargo has this yet but perhaps i should write an rfc for it. i imagine we could write some logic to do this ourselves as well.

@ashleygwilliams ashleygwilliams added the question Further information is requested label Apr 27, 2018
@mgattozzi
Copy link
Contributor

I wanted to see if there was a way to see if we can see a discrepency in versions somehow there is:

michael@eva-01 ~ % cargo install wasm-bindgen-cli
    Updating registry `https://github.com/rust-lang/crates.io-index`
 Downloading wasm-bindgen-cli v0.2.6
  Installing wasm-bindgen-cli v0.2.6
error: binary `wasm-bindgen` already exists in destination as part of `wasm-bindgen-cli v0.2.4`
binary `wasm2es6js` already exists in destination as part of `wasm-bindgen-cli v0.2.4`
Add --force to overwrite

Now the solution to "Should we add -f" will be to parse this output and compare semver, possibly with a crate of some sort. Not gonna lie this is ugly and something we'll need to invoke on every call but it's better than nothing. If we go forward with this we should tell the user they'll always need to use the latest version, possibly warning them in the process of doing so.

@alexcrichton
Copy link
Contributor

FWIW I definitely feel like Cargo should have a feature for this. In the meantime I'd recommend keeping up to date with the latest wasm-bindgen version if possible, even just tonight the latest Rust nightly broke wasm-bindgen and I had to publish a fix.

Hopefully soon everything will be stable and breakage will be a thing of the past!

@mgattozzi
Copy link
Contributor

@alexcrichton Ashley and I were talking, probably going to write up an RFC for this so you can do something like cargo install wasm-bindgen-cli@latest or something like that. In the meantime I'm going to start a cargo subcommand proof of concept that works with crates.io api to make it nicer

@alexcrichton
Copy link
Contributor

Sounds great!

@Pauan
Copy link
Contributor

Pauan commented Apr 27, 2018

A dedicated command would be nice, but wouldn't cargo install wasm-bindgen && cargo update --package wasm-bindgen be good enough?

(It would probably have to be run as two commands, and errors from the first command ignored, but...)

@mgattozzi
Copy link
Contributor

mgattozzi commented Apr 27, 2018

michael@eva-01 ~ % cargo update --package wasm-pack-cli
error: could not find `Cargo.toml` in `/home/michael` or any parent directory

Nope doesn't work for CLI tools :(

@steveklabnik
Copy link
Contributor

FWIW I have always found @latest to be massively confusing, but also very much agree that something should be done here.

@Pauan
Copy link
Contributor

Pauan commented Apr 27, 2018

@mgattozzi Ah, that's a real shame. I guess it'll require changes to cargo then.

@itsybitesyspider
Copy link
Contributor

I feel like the developer experience will be much improved if I can lock wasm-pack and wasm-bindgen to a specific known-working version. Updates break things, and I'm not sure that simply having wasm-bindgen installed at all is something I really need help with from wasm-pack. I'm able to lock everything else in my development environment (including rust nightly).

It's too bad cargo (AFAIK) can't install binaries per-project like npm does.

@steveklabnik
Copy link
Contributor

Could wasm-bindgen export a library and then wasm-pack could use that instead? Then it’d be easier to lock, and no install needed.

@alexcrichton
Copy link
Contributor

FWIW there's a wasm-bindgen-cli-support crate on crates.io which is the library that wasm-bindgen-the-CLI uses. That being said I'm not currently maintaining it for semver compatibility, so I wouldn't recommend using it right now. If that's the chosen option to go here, though, I'd be more than willing to put some effort into cleaning it up and providing semver stability guarantees!

@data-pup
Copy link
Member

data-pup commented May 2, 2018

This issue was brought up in #81, so I figured I'd pop in to say if there's anything that I can do to help with the cleaning up the wasm-bindgen-cli-support crate, let me know! I'd love to help out :)

@mgattozzi
Copy link
Contributor

So I was discussing this with @ashleygwilliams and it looks like the cargo-update tool has it's internal library available and the binaries for cargo-update use that. Essentially we just need to use that to check local versions compare it with the one on crates.io and go from there. I'll get started on this, but it'll let us avoid force installing unless there's a newer version. I think this is a great start.

@ashleygwilliams ashleygwilliams added to-do stuff that needs to happen, so plz do it k thx PR attached there's a PR open for this issue and removed question Further information is requested labels May 29, 2018
@ashleygwilliams ashleygwilliams added this to the 0.3.0 milestone May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR attached there's a PR open for this issue to-do stuff that needs to happen, so plz do it k thx
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants