Use Yarn to manage dependencies #728

stephanethomas · 2016-10-11T16:36:25Z

This pull request updates Delphin to use Yarn to manage dependencies. As mentioned here, Yarn is a fast, reliable, and secure alternative Npm client. It has several advantages over Npm such as offering an offline mode, and better network performance. Right now it divides the installation process by almost two on the first build. This process even goes from 26 seconds to less than half a second on the next builds!

Npm

First build

time npm install

[...]

real    2m10.855s
user    1m13.612s
sys     0m13.112s

Next builds

time npm install

[...]

real    0m26.348s
user    0m25.052s
sys     0m0.660s

Yarn

First build

time yarn install --force

[...]

real    1m22.335s
user    0m27.444s
sys     0m8.820s

Next builds

time yarn install

yarn install v0.15.1
success Already up-to-date.
Done in 0.20s.

real    0m0.412s
user    0m0.384s
sys     0m0.020s

Testing instructions

Run npm install -g yarnpkg to install Yarn
Run git checkout test/yarn and start your server
Check that everything works as before

Additional notes

CircleCi still uses npm for building artifacts.

Reviews

Code
Product

@Automattic/sdev-feed

drewblaisdell · 2016-10-11T21:14:28Z

This is awesome.

We no longer need npm run qs as it saves < one second now. :) I added a commit to remove it.

I was able to get it running on Circle by adding yarn to the dependencies in package.json. This way, Circle installs Yarn and then use it in npm run prod:static without failing. Feel free to remove the commit that tests this by editing circle.yml and squash the others.

It'd be good to have one more dev's opinion before merging this I think, but this works well for me and the code looks good.

stephanethomas · 2016-10-12T08:58:40Z

Note we will have to update the readme file to mention this new dependency.

scruffian · 2016-10-12T09:16:27Z

Note we will have to update the readme file to mention this new dependency.

Done! Uncharacteristically I had already thought of it :)

stephanethomas · 2016-10-12T09:35:29Z

README.md

-3. Execute `npm start` to install packages and start the server
-4. Add `127.0.0.1 delphin.localhost` to your `hosts` file
-5. Open http://delphin.localhost:1337 in your browser
+3. Install Webpack: `npm install webpack -g`


I don't think we have to install webpack explicitly. Yarn will take care of that.

stephanethomas · 2016-10-12T09:52:40Z

package.json

@@ -9,16 +9,15 @@
    "build:server": "webpack --config webpack.server.config.js --progress --colors",
    "build:static": "BUILD_STATIC=true REFRESH_I18N_CACHE=true node server/build/bundle",
    "clean": "rm -f public/scripts/vendor.*.js && rm -f public/scripts/*.bundle.*.js && rm -f public/scripts/bundle.*.js && rm -f public/styles/bundle.*.css && rm -f public/scripts/assets.json && rm -rf server/build && npm run clean:i18n-cache && npm run clean:static",
-    "clean:all": "rm -rf node_modules && npm run clean",
+    "clean:all": "rm -rf node_modules && rm -f yarn.lock && npm run clean",


We should probably not remove the yarn.lock file since it's basically an extension of package.json.

Tug · 2016-10-12T11:53:31Z

Awesome indeed.
However we can't update our scripts commands to use yarn just yet. It's still a bit young and fails for many of our commands. Let's follow this issue which reports the same bug yarnpkg/yarn#733

So we'll have to use both yarn and npm for some time.

I added a simple commit to make circleci install deps using yarn instead of npm.

gziolo · 2016-10-12T11:55:24Z

yarn run is not supported yet :D

Tug · 2016-10-12T13:10:35Z

circle.yml

 deployment:
  production:
-    branch: master
+    branch: /.*/


we will need to revert this to master before merging

gziolo · 2016-10-12T13:33:48Z

Still fighting with CircleCI? :D

Tug · 2016-10-12T13:40:39Z

@gziolo Indeed :)

I have found a couple more issues with yarn:

if node_modules is altered (for instance you remove a dependency) yarn won't see it, you need to reinstall everything.
It seems postinstall works for dependencies (for instance node-sass binaries are being built locally) but it seems to be failing in circle-ci, I'm still investigating but it might be a cache problem.

Tug · 2016-10-12T13:59:35Z

npm cache clean on circle-ci did the trick \o/

I'm still a bit worried about how it will play out if we upgrade node for instance, yarn might not rebuild the dependencies. Maybe that's an issue for another time.

This is ready for review ;)

gziolo · 2016-10-12T14:54:09Z

I would also recommend investigating build time on Circle CI:

Comparing for the times shared by @stephanethomas, it doesn't looks like an improvement ... Maybe it still needs some tweaks :)

fabianapsimoes · 2016-10-12T14:57:46Z

Nice experiment, @stephanethomas! Also, good team work getting this ready to ship 🍰

This doesn't seem to break any of the main flows, so apart from any further performance tweaks, product 👍

Tug · 2016-10-12T15:13:02Z

it doesn't looks like an improvement

That's probably because I cleared the cache for this test.

I think it's an improvement for our dev environment at least, it's a pain to wait 20s for npm to detect than nothing has changed :)

gziolo · 2016-10-12T15:18:42Z

I took a closer look at Circle CI. Everything looks good, you just enabled deployment part for branches other than master and that increased the build time:

So I guess it looks good to go, unless @stephanethomas has some comments :)

gziolo · 2016-10-12T15:28:55Z

One more comment. For some reasons npm test & npm run lint was executed twice on Circle CI:

On all other branches it was executed only once:

Tug · 2016-10-12T15:47:35Z

Yep because I added those in our circle.yml here 64a5303
The idea is to remove it from circleci config once this is merged

stephanethomas · 2016-10-12T16:33:33Z

circle.yml

+dependencies:
+  pre:
+    - curl -o- -L https://yarnpkg.com/install.sh | bash
+    - yarn run preinstall


Can't we simply do npm install -g yarnpkg like in development?

We can but the idea is to get rid of npm eventually

Gotcha. It's just that I'm not a big fan of downloading and executing a shell script :).

Agreed it's not the best, but in the end, we do have to trust some server to give us the right version of our library anyway. Granted there are more secure solution on the market!

stephanethomas · 2016-10-12T16:41:52Z

README.md

-3. Execute `npm start` to install packages and start the server
-4. Add `127.0.0.1 delphin.localhost` to your `hosts` file
-5. Open http://delphin.localhost:1337 in your browser
+4. Install yarn: `npm install -g yarnpkg`


Shouldn't we simply mention it in point #1 instead?

1. Make sure you have `git`, `node`, `npm`, and [`yarn`](https://yarnpkg.com/en/docs/install) installed

I think it's better to point to Yarn's installation page since the process is different depending on the platform.

Pushed a commit to do just that ;)

Tug · 2016-10-12T22:48:04Z

Product was 👍 , I removed the commit which let us build an archive version of our app for other branches than master.

This moves commands specified in the CircleCi dashboard to this configuration file. Note these commands replace the ones inferred by CircleCi.

Come on, we're no longer in beta!

stephanethomas · 2016-10-14T13:01:11Z

I've rebased and cleaned the commits history.

stephanethomas added the framework label Oct 11, 2016

stephanethomas self-assigned this Oct 11, 2016

stephanethomas added the [status] needs review label Oct 11, 2016

stephanethomas force-pushed the test/yarn branch from 94f507c to 52f46f3 Compare October 11, 2016 17:15

stephanethomas commented Oct 12, 2016

View reviewed changes

stephanethomas force-pushed the test/yarn branch from 1702ec8 to 57613d7 Compare October 12, 2016 09:50

stephanethomas commented Oct 12, 2016

View reviewed changes

Tug force-pushed the test/yarn branch 2 times, most recently from 1d9b4a8 to 633266f Compare October 12, 2016 11:27

Tug force-pushed the test/yarn branch from 633266f to 283923f Compare October 12, 2016 11:55

Tug force-pushed the test/yarn branch from b2ce12e to 5dbf121 Compare October 12, 2016 13:08

Tug reviewed Oct 12, 2016

View reviewed changes

circle.yml

deployment:

production:

branch: master

branch: /.*/

Copy link

Contributor

Tug Oct 12, 2016

Choose a reason for hiding this comment

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

we will need to revert this to master before merging

Tug force-pushed the test/yarn branch from 933135f to 5dbf121 Compare October 12, 2016 13:46

stephanethomas commented Oct 12, 2016

View reviewed changes

Tug force-pushed the test/yarn branch from 1d8d095 to 371aec5 Compare October 12, 2016 22:45

stephanethomas added 4 commits October 14, 2016 14:36

Specify test commands for CircleCi

a4ece44

This moves commands specified in the CircleCi dashboard to this configuration file. Note these commands replace the ones inferred by CircleCi.

Bump Delphin version

0216ed9

Come on, we're no longer in beta!

Fix invalid dependencies

c7113c8

Use Yarn to manage dependencies

64b2f9f

stephanethomas force-pushed the test/yarn branch from 371aec5 to 64b2f9f Compare October 14, 2016 12:59

stephanethomas merged commit 0fc411a into master Oct 14, 2016

stephanethomas deleted the test/yarn branch October 14, 2016 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Yarn to manage dependencies #728

Use Yarn to manage dependencies #728

stephanethomas commented Oct 11, 2016 •

edited

Loading

drewblaisdell commented Oct 11, 2016

stephanethomas commented Oct 12, 2016

scruffian commented Oct 12, 2016

stephanethomas Oct 12, 2016

stephanethomas Oct 12, 2016

Tug commented Oct 12, 2016

gziolo commented Oct 12, 2016

Tug Oct 12, 2016

gziolo commented Oct 12, 2016

Tug commented Oct 12, 2016 •

edited

Loading

Tug commented Oct 12, 2016

gziolo commented Oct 12, 2016

fabianapsimoes commented Oct 12, 2016

Tug commented Oct 12, 2016 •

edited

Loading

gziolo commented Oct 12, 2016 •

edited

Loading

gziolo commented Oct 12, 2016

Tug commented Oct 12, 2016

stephanethomas Oct 12, 2016

Tug Oct 12, 2016

stephanethomas Oct 12, 2016

Tug Oct 12, 2016

stephanethomas Oct 12, 2016

Tug Oct 12, 2016

Tug commented Oct 12, 2016

stephanethomas commented Oct 14, 2016

Use Yarn to manage dependencies #728

Use Yarn to manage dependencies #728

Conversation

stephanethomas commented Oct 11, 2016 • edited Loading

Npm

First build

Next builds

Yarn

First build

Next builds

Testing instructions

Additional notes

Reviews

drewblaisdell commented Oct 11, 2016

stephanethomas commented Oct 12, 2016

scruffian commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tug commented Oct 12, 2016

gziolo commented Oct 12, 2016

Choose a reason for hiding this comment

gziolo commented Oct 12, 2016

Tug commented Oct 12, 2016 • edited Loading

Tug commented Oct 12, 2016

gziolo commented Oct 12, 2016

fabianapsimoes commented Oct 12, 2016

Tug commented Oct 12, 2016 • edited Loading

gziolo commented Oct 12, 2016 • edited Loading

gziolo commented Oct 12, 2016

Tug commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tug commented Oct 12, 2016

stephanethomas commented Oct 14, 2016

stephanethomas commented Oct 11, 2016 •

edited

Loading

Tug commented Oct 12, 2016 •

edited

Loading

Tug commented Oct 12, 2016 •

edited

Loading

gziolo commented Oct 12, 2016 •

edited

Loading