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

Move from npm cli -> yarn #457

Merged
merged 8 commits into from
Nov 11, 2016
Merged

Move from npm cli -> yarn #457

merged 8 commits into from
Nov 11, 2016

Conversation

steveklabnik
Copy link
Member

yarn is a new cli for the npm registry from Facebook, Google, and Tilde.
It's patterned after Cargo, and @wycats was involved in making it. The
big things we care about here:

  1. It is faster. 30s vs 45 for npm cli on my mcahine.
  2. It has a lockfile. Cargo has a lockfile. Lockfiles are good.

This commit adds a yarn.lock, and fixes our .travis.yml to use yarn
instead.

@rwjblue
Copy link

rwjblue commented Oct 11, 2016

Should likely change (in .travis.yml):

 cache:
   directories:
     - node_modules

To:

 cache:
   directories:
     - $HOME/.yarn-cache

Yarn symlinks from ~/.yarn-cache into node_modules (using as flat a structure as is possible), so caching that instead of node_modules (which is basically just symlinks AFAICT?) seems better and likely faster.

@steveklabnik
Copy link
Member Author

@rwjblue Ah thanks! I wasn't aware. 👍

@steveklabnik
Copy link
Member Author

Interesting, Travis seems to be failing because I have a newer node locally. Might as well fix that too; I was planning on doing it, but in a different PR.

@rwjblue
Copy link

rwjblue commented Oct 11, 2016

Also, yarn does not support Node 0.12, so you will need to update to at least 4.0 (I'd say 6.0 is probably better since it will be next LTS). I'm looking in the Travis docs for what the syntax is for that (since the declared language is rust)

@steveklabnik
Copy link
Member Author

@rwjblue just pushed a new commit with that 😄

@steveklabnik
Copy link
Member Author

I had asked @alexcrichton what it would take to upgrade node, given that we're using a buildpack, https://github.com/tonycoco/heroku-buildpack-ember-cli/blob/master/bin/compile#L36-L37 should Just Do It.

@steveklabnik
Copy link
Member Author

Hm, checking why this failed this time, and didn't last time...

@steveklabnik
Copy link
Member Author

... i reorganized these commits so that the node upgrade happens first, but github seems to still show them out of order. It looks like travis is testing the second commit, which is accurate, though?

Anyway, I am 99% sure this will pass, and so is now ready for review.

yarn is a new cli for the npm registry from Facebook, Google, and Tilde.
It's patterned after Cargo, and @wycats was involved in making it. The
big things we care about here:

1. It is faster. 30s vs 45 for npm cli on my mcahine.
2. It has a lockfile. Cargo has a lockfile. Lockfiles are good.

This commit moves development over to yarn.

Some changes:

Instead of needing to `npm install -g` things, we can instead include
them in our `package.json`, and use `yarn run`. This doesn't pollute the
global environment, which is a win!

The README and .travis.yml are updated accordingly.
@steveklabnik
Copy link
Member Author

I have no idea what Travis's beef is here, this runs just fine locally for me. gah.

yarn is a new cli for the npm registry from Facebook, Google, and Tilde.
It's patterned after Cargo, and @wycats was involved in making it. The
big things we care about here:

1. It is faster. 30s vs 45 for npm cli on my mcahine.
2. It has a lockfile. Cargo has a lockfile. Lockfiles are good.

This commit moves development over to yarn.

Some changes:

Instead of needing to `npm install -g` things, we can instead include
them in our `package.json`, and use `yarn run`. This doesn't pollute the
global environment, which is a win!

The README and .travis.yml are updated accordingly.
@ducks ducks mentioned this pull request Oct 11, 2016
@ducks
Copy link

ducks commented Oct 11, 2016

Seems to maybe be a side effect of yarnpkg/yarn#733 perhaps.

@steveklabnik, I sent you a PR to add an ember script, which I think might fix it.

@steveklabnik
Copy link
Member Author

Oh interesting. I just switched travis to call ember test rather than going through npm run test, but that might be better. Lemme check it out.

@steveklabnik
Copy link
Member Author

@rjgoldsborough looks like that didn't work.

@ducks
Copy link

ducks commented Oct 11, 2016

Ah darn. Sounds like calling ember directly is the best fix for now.

@steveklabnik
Copy link
Member Author

Okay! @carols10cents @alexcrichton what do you think?

@ducks
Copy link

ducks commented Oct 11, 2016

I do wonder if that'll pop up when trying to run too.

But it doesn't happen locally so it's hard to tell.

@carols10cents
Copy link
Member

So I pulled down this branch locally and followed the instructions in the readme. Specifically, I ran:

npm install -g yarn
yarn
yarn run bower install

Those seemed to work fine, then I did this:

 $ yarn run start:staging
yarn run v0.15.1
$ "yarn run ember server --proxy https://staging-crates-io.herokuapp.com"
sh: yarn run ember server --proxy https://staging-crates-io.herokuapp.com: No such file or directory
error Command failed with exit code 127.
info Visit http://yarnpkg.com/en/docs/cli/run for documentation about this command.

Am I doing something wrong? Is it the same issue yinz linked to?

@steveklabnik
Copy link
Member Author

Ugh, okay. So yeah, something is the matter with npm scripts here. I think it's the issue we linked to. We'll have to wait for that to get cleared up, I think.

@ducks
Copy link

ducks commented Oct 12, 2016

Yeah I think that's still the issue.

I know with npm run you used to have to separate the command and arguments with --.
https://docs.npmjs.com/cli/run-script

Maybe try changing the start:staging command to run yarn run ember -- server --proxy https://staging-crates-io.herokuapp.com

I tried it both ways locally and got different results I think

yarn run v0.15.1
$ yarn run ember server --proxy https://staging-crates-io.herokuapp.com
yarn run v0.15.1
$ /Users/ducks/devel/rust/crates.io/node_modules/.bin/ember server

vs

yarn run v0.15.1
$ yarn run ember -- server --proxy https://staging-crates-io.herokuapp.com
yarn run v0.15.1
$ /Users/ducks/devel/rust/crates.io/node_modules/.bin/ember server --proxy https://staging-crates-io.herokuapp.com

@ducks
Copy link

ducks commented Oct 12, 2016

It looks like there are a few known issues with that too for now

yarnpkg/yarn#616
yarnpkg/yarn#864

@steveklabnik
Copy link
Member Author

@rjgoldsborough was just about to say the same yarnpkg/yarn#896, probably best to hold off. cool.

@alexcrichton
Copy link
Member

@steveklabnik is this ready to go? Or perhaps still waiting for a yarn point release?

@steveklabnik
Copy link
Member Author

I believe that it was released, let me kick off another Travis build to check.

@steveklabnik
Copy link
Member Author

Looks like travis is green now 🎊

I would like to double check that the issues @carols10cents had are fixed; I can run those commands tomorrow, but I think they should Just Work now.

@carols10cents
Copy link
Member

I updated yarn to v0.16.1 and I was able to run the setup steps, run just the frontend proxying to staging, run both kinds of tests, and run the backend locally and the front end pointing to that! 🎉 🎊

One nit though: Line 32 in the README still says "npm start".

@alexcrichton
Copy link
Member

@steveklabnik I remember talking about this but I forget the resolution, do you know if this'll work on heroku deployments as well? Do we need to switch buildpacks to a yarn-enabled buildpack?

@steveklabnik
Copy link
Member Author

@alexcrichton ah, so we talked about it for the node upgrade; that def. works with the existing buildpack. That is, the buildpack looks at this line to install a node version. But it still uses npm for actually building.

In theory, it would still be fine to just ship this; this doesn't break npm from working. And then we could work out the buildpack issues. But, the more conservative choice is to hold off until that's good to go as well. I've opened tonycoco/heroku-buildpack-ember-cli#139 to see what the buildpack's maintainers think about adding support.

@steveklabnik
Copy link
Member Author

So, given that the buildpack author has not replied, how should I proceed? Should I just fork and add it and we can use my fork?

@alexcrichton
Copy link
Member

Well that's only mildly disturbing... @samphippen out of curiosity are you familiar if there's a "best practice" for deploying ember to Heroku? If so, is it still the buildpack at https://github.com/tonycoco/heroku-buildpack-ember-cli?

@fables-tales
Copy link

Honestly, I'd support forking and running off a version that supports yarn. Buildpacks are the sort of thing that you're not likely to change often going forward. Also: having production usage would be a good reason for the original authors to merge. 🚀

@alexcrichton
Copy link
Member

Ok, thanks! @steveklabnik if you'd like to fork I'd be cool merging!

@steveklabnik
Copy link
Member Author

I'll try to get to it soon!

@steveklabnik
Copy link
Member Author

Sooooooooooooooooooooooooo six days ago tonycoco/heroku-buildpack-ember-cli@01f2e35

But the "official" one is still experimental. So. I don't even know.

@alexcrichton
Copy link
Member

Eh if it's the same code it's what we've been using all along, so that should be fine.

@steveklabnik
Copy link
Member Author

@ryanponce wants to take a shot at this, so I'm gonna work with them on it

@steveklabnik
Copy link
Member Author

Relevant to our interests heroku/heroku-buildpack-nodejs#337 (comment)

@alexcrichton
Copy link
Member

Awesome! @steveklabnik want to include that here and I'll test it out in staging?

@steveklabnik
Copy link
Member Author

Yeah, gonna take a look at it soon. @ryanponce did you get a chance to play with any of this the other day?

@ryanponce
Copy link

@steveklabnik Ah not yet. You guys go for it, I'll pick up another one.

@ducks
Copy link

ducks commented Nov 11, 2016

I'd love to help finish the move to yarn so just holler if you don't wanna bother with it. :)

I'll admit I don't know a whole lot about heroku but looks like this might be added with thebuildpacks:addcommand?

@alexcrichton
Copy link
Member

I believe the only change necessary is to .buildpacks in this repo, and that'll propagate naturally

@ducks
Copy link

ducks commented Nov 11, 2016

Cool. I sent @steveklabnik a PR that adds that.

Thanks.

adding yarn based nodejs buildpack
@steveklabnik
Copy link
Member Author

I have since merged the PR, let's give it a try!

@alexcrichton alexcrichton merged commit 09ba5bc into rust-lang:master Nov 11, 2016
@alexcrichton
Copy link
Member

Deployed!

@steveklabnik steveklabnik deleted the yarn branch November 12, 2016 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants