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

doc: Document installation via npm or yarn #767

Merged
merged 3 commits into from
Jan 28, 2020

Conversation

drager
Copy link
Member

@drager drager commented Jan 20, 2020

Closes #751.

Looks like this:

Screenshot from 2020-01-20 20-41-29

@ashleygwilliams ashleygwilliams added this to the 0.9.0 milestone Jan 22, 2020
Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

@drager i think this looks great! we may decide we want to promote this as the primary way to install wasm-pack but i dont think we need to block release on it

Install using <b>npm</b> or <b>yarn</b>:
</p>
<code>
npm install -g wasm-pack
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these should be installed globally. To ensure that builds are reproducible, they should instead be local, like this:

npm install --save-dev wasm-pack
yarn add --dev wasm-pack

Copy link
Member

Choose a reason for hiding this comment

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

in general i would agree with you but this release bakes cargo-generate into wasm-pack.. so now it can also generate projects, which i think means it should be globally installed

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the generation be changed to install wasm-pack locally, then? There are a lot of benefits to local packages, and some definite long term pain with global packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

(We don't necessarily need to change it in this release, we can change the generation behavior in the next release.)

Copy link
Member

Choose a reason for hiding this comment

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

generation means, cargo-generate, and uses the flow espouse by our docs, replacing the step of generating a project via cargo generate with wasm-pack new. this was proposed and approved and ships as part of this release. we are not going to change it because that would be a breaking change.

i think we can advise people to install it globally. folks who have a project where it is better suited to install as a dev dep likely are aware of that. i tend to prefer to give instructions to folks who are more beginner, than folks with more advanced needs.

Copy link
Member

@ashleygwilliams ashleygwilliams Jan 25, 2020

Choose a reason for hiding this comment

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

i am ok recommending npx, but the dev dep suggestion doesn't fully work- as it only accounts for a subset of the potential usecases.

Copy link
Member

Choose a reason for hiding this comment

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

ok here's my proposal:

Using npm:

$ npx wasm-pack new my-project to generate a new project, and
$ npm i wasm-pack --save-dev to build and publish

... yarn doesn't seem to have an equivalent that works (dlx doesn't work on wasm-pack today)

would this work for both of you, @Pauan and @drager ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the dev dependency does work for all use cases. Here's an example: you can use npx @angular/cli new foo to create a new Angular app. This will do the following:

  1. It creates a new folder foo

  2. It puts all the necessary Angular glue into foo, including a package.json which contains @angular/cli as a devDependencies

  3. Then it automatically runs npm install

In other words, npx @angular/cli new creates a new project which contains @angular/cli in devDependencies. So you're using the global package to create a new project which contains the local package.

This is the normal way of doing things, create-react-app does the same thing. They even explicitly recommend not installing create-react-app globally.

This has some advantages:

  • When creating a new project, npx will always automatically use the most up to date version, so the user no longer needs to manage the version of wasm-pack.

  • It installs wasm-pack locally, thus avoiding all of the issues with global installs.

  • The user doesn't need to worry about the difference between global and local, they can just use npx wasm-pack and everything will work, both globally and locally.

As for yarn, the equivalent would be yarn create wasm-pack new and yarn wasm-pack. Unfortunately they don't use the same command for both, but we can just explain that in the docs.

Copy link
Member

Choose a reason for hiding this comment

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

i gave a proposal that sounds like what you are suggesting, can you make an alternate proposal or comment on mine, i cannot actually tell what you are suggesting.

Copy link
Contributor

Choose a reason for hiding this comment

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

The difference is that with your proposal the user has to manually install wasm-pack locally, but with my proposal wasm-pack new will automatically install wasm-pack locally.

@ashleygwilliams
Copy link
Member

ashleygwilliams commented Jan 26, 2020 via email

@Pauan
Copy link
Contributor

Pauan commented Jan 26, 2020

@ashleygwilliams Sure, that's why I was saying we can change the behavior in the next release, since it's a non-breaking change.

Copy link
Contributor

@Pauan Pauan left a comment

Choose a reason for hiding this comment

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

I'm going to approve this, under the assumption that after the release we'll change it to use local install.

@ashleygwilliams ashleygwilliams merged commit 9a9aca7 into rustwasm:master Jan 28, 2020
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.

Document npm installer
3 participants