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

Change NPM with Yarn #764

Closed
wants to merge 8 commits into from
Closed

Change NPM with Yarn #764

wants to merge 8 commits into from

Conversation

DanReyLop
Copy link
Contributor

Folks at Calypso tried this a few months ago but shelved it because Yarn wasn't mature enough. Relevant PR: Automattic/wp-calypso#8660

Many big JS projects are already using it (including Webpack and React, for example), so I guess that's mature enough to give it a try.

Yarn is a JS package manager created by Facebook. It can be used as an (almost) drop-in replacement for NPM. Its advantages over NPM are:

  • It has a locking mechanism built-in. Similar in concept to shrinkwrap, but it's used by default, so there's no way to install a package and forget to update npm-shrinkwrap.json or something like that.
  • It's faster. Much faster. Expect installing woocommerce-connect-client from scratch to be twice as fast, or more.
  • It mantains a local packages cache so it has some form of offline operation.
  • It has a deterministic install alrogithm, as opposed to NPM3.
  • Some cool new functionallity. For example: yarn why packageName will show why that package is installed (which dependency or sub-sub-subdependency is using it).
  • It has more emojis.
  • More info: https://yarnpkg.com/

To install Yarn: https://yarnpkg.com/en/docs/install#mac-tab

While I was at it, I re-arranged the dependencies and devDependencies, as we were being quite erratic deciding where to put each one. Since all the code is bundled, there are no real "dependencies" per-se, so I made an executive decision: If something is needed to run npm run i18n and npm run release, then it's a dependency, else it's a devDependency. That leaves as devDependencies testing-related stuff, linters and things like webpack-dev-server. This change can be back-ported to master even if we discard the rest of this PR.You can disagree with that, of course, as long as you provide another definition of dependency and devDependency :)

To test:

  • Install Yarn.
  • Run yarn (shortcut to yarn install).
  • Check that everything works. The easiest way to do that is to run the tests and generate the release bundle, that should cover 99% of our code paths.
  • To test that the dependencies are enough to build a release, remove node_modules and yarn.lock, then run yarn install --production && npm run release . It should work.

Note: In Windows, yarn has a race condition that affects the karma CLI. More details. This shouldn't be a gating issue IMHO.

Daniel Rey Lopez added 5 commits January 2, 2017 19:12
From now on, everything needed to run "npm run release" and "npm run i18n
 are "dependencies", even if they don't end up in the bundle (i.e: babel).
Everything else (redbox, testing-related deps, eslint, etc) are devDependencies.
@DanReyLop
Copy link
Contributor Author

DanReyLop commented Jan 4, 2017

Marked as "in progress" until this issue is fixed: sass/node-sass#1804 (of course, to make things more interesting, it only happens in Travis CI)

@DanReyLop
Copy link
Contributor Author

Nevermind, I found a workaround: not cache node_modules between builds, and cache ~/.yarn-cache instead.

@v18
Copy link
Contributor

v18 commented Jan 13, 2017

Started reviewing this, but I didn't finish everything I wanted to do (getting yarn to work out of the box & manual testing), which I'll get to next week. Here are my notes so far:

The changes from npm to yarn seem thorough at first pass. After installing yarn, I tried to run yarn, but ran into this issue:

wp-calypso@0.17.0: The engine "node" is incompatible with this module. Expected version "6.9.1".

This is likely an issue with using nvm to manage my node versions.

Using the usually-recommended workaround yarn --ignore-engines installs the packages, and gives me this warning:

warning Incorrect peer dependency "react@^15.4.1".

It took 56.05s.

All of the unit tests passed ✅ I have not yet tested it manually, though.

I agree with your devDep/dep definition.

@DanReyLop
Copy link
Contributor Author

The changes from npm to yarn seem thorough at first pass. After installing yarn, I tried to run yarn, but ran into this issue:
wp-calypso@0.17.0: The engine "node" is incompatible with this module. Expected version "6.9.1".
This is likely an issue with using nvm to manage my node versions.

That was an oversight on my part, I forgot to update the .nvmrc. Done.

warning Incorrect peer dependency "react@^15.4.1".

Yep, we have some nasty stuff there. We can't update to react@15.4 because we use an unsupported internal API that was removed (this one) but due to some transitive dependencies we end up importing something (I think related to i18n-calypso) that has a peerDependency with react@15.4. Everything works and it's just a warning, so I would say this is fine for now.

@v18
Copy link
Contributor

v18 commented Jan 19, 2017

That was an oversight on my part, I forgot to update the .nvmrc. Done.

Thanks! It works now with yarn

To test that the dependencies are enough to build a release, remove node_modules and yarn.lock, then run yarn install --production && npm run release . It should work.

I ran into this issue when running yarn install --production (it works with npm):

Error: Cannot find module 'abbrev'

It looks like the issue we're running into is yarnpkg/yarn#761 - which seems to be being fixed atm. Until it's fixed, though, I say we shouldn't merge this PR.

If we end up using yarn, we should decide on a firm yarn version (i.e. replace 0.18.1+ from the docs with a specific version number) - different yarn versions will lead to different yarn.lock files and may cause trouble if every committer is not using the same version

I am slightly in favour of not switching to yarn because of these concerns:

  • Our server and client will be running different package mangers - should we use yarn on the server, too? An example of the cognitive strain we may experience: forgetting about shrinkwrap happens, and I imagine it'll happen even more often when one of our repos requires it and the other does not.
  • We have not (afaik) run into performance issues (npm takes only ~1-2 minutes longer to install) or shrinkwrap issues. Using yarn, given its newness, seems like solving for a problem we don't have yet and is low priority
  • Given that yarn is new and may cause unforeseen trouble in the short-term, and the switch is low priority, it doesn't seem to be worth the effort of the team to switch

Happy to hear a second opinion, though :)

@DanReyLop
Copy link
Contributor Author

It looks like the issue we're running into is yarnpkg/yarn#761 - which seems to be being fixed atm. Until it's fixed, though, I say we shouldn't merge this PR.

Doesn't happen to me, so I assumed we were fine. So much for determinism... According to the issue comments, this may or may not be solved. Could you try using the latest yarn version? 0.19.1 at this moment. I wholeheartedly agree we shouldn't switch until we can be sure it's stable.

If we end up using yarn, we should decide on a firm yarn version (i.e. replace 0.18.1+ from the docs with a specific version number) - different yarn versions will lead to different yarn.lock files and may cause trouble if every committer is not using the same version

Once yarn reaches 1.0.0, the yarn.lock file format should be considered frozen for all 1.x.x versions (semver yay!), so at that point we should be fine. However, FB is heavily pushing for yarn, it's already used in very big projects, and we aren't using it in any weird edge cases, so I think new versions will introduce only backwards-compatible changes. Probably reasonable to wait until 1.0.0 though.

Our server and client will be running different package mangers - should we use yarn on the server, too? An example of the cognitive strain we may experience: forgetting about shrinkwrap happens, and I imagine it'll happen even more often when one of our repos requires it and the other does not.

+1 to that. I was aiming to switch in one repo, and if everything goes fine, switch in all other repos. If yarn works fine for a heavy project like the client, there's no reason to not use it everywhere else.

We have not (afaik) run into performance issues (npm takes only ~1-2 minutes longer to install) or shrinkwrap issues. Using yarn, given its newness, seems like solving for a problem we don't have yet and is low priority

I agree it's not super-high priority (this is a try/ branch after all). Extra performance is nice, but it's not the main problem yarn solves. We have had problems with shrinkwrap. Until a few versions of npm ago, it was broken in many ways, to the point we were hand-editing it sometimes. The fact that we have a script called npm run update-deps that nukes everything and regenerates the shrinkwrap file from scratch kind-of hints at its reliability :)

One of the big selling points of yarn is that it uses the yarn.lock file by default, so there's no chance of package.json and the lock file getting out of sync (except if you hand-edit package.json, we should probably add a hook or CI script to prevent that, it should be easily detectable).

So this is probably not going to happen right now, but I think we at least should keep this in the back of our heads.

@v18
Copy link
Contributor

v18 commented Jan 30, 2017

Could you try using the latest yarn version? 0.19.1 at this moment.

Works for me now, and the issue from earlier is marked as resolved, too.

+1 to that. I was aiming to switch in one repo, and if everything goes fine, switch in all other repos.

Ah, nice. Yeh, I agree with that.

We have had problems with shrinkwrap. Until a few versions of npm ago, it was broken in many ways, to the point we were hand-editing it sometimes.

Ugh, that's annoying. Thanks for pointing it out - definitely a good consideration. Maybe even good to revisit yarn the next time we have an issue.

So this is probably not going to happen right now, but I think we at least should keep this in the back of our heads.

Definitely agree there. Let's revisit at version 1 at the very latest?

@DanReyLop DanReyLop closed this Jun 13, 2017
@bor0 bor0 deleted the try/yarn branch November 23, 2019 23:22
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.

2 participants