Using yarn instead of npm

[squadette]

Here is preliminary pull-request for using yarn. It seems to be passing tests on my machine (the same as for vanilla npm).

Currently it fails in Travis because of yarnpkg/yarn#683 (comment)

I'm going to add --mutex network and try again. Please test this in your environments.

NB: first two commits are mostly written by @justin808, but were improved/fixed by me. Justin, please feel free to adjust attribution before merging however you wish.

Thanks,

---

This change is 

[justin808]

Lots of comments. Good start!

---

Reviewed 54 of 54 files at r1.
Review status: 50 of 54 files reviewed at latest revision, 9 unresolved discussions, some commit checks broke.

---

.travis.yml, line 9 at r1 (raw file):

  - 2.1.10
  - 2.2.6
  - 2.3.1

should not be removed

we want to support older versions of ruby and also ruby 2.4!

---

CONTRIBUTING.md, line 99 at r1 (raw file):

When you use a relative path, be sure to run the above yarn command whenever you change the node package for react-on-rails.

Please double check on if there is a yarn link that we can use.

---

package.json, line 59 at r1 (raw file):

    "lint": "yarn run eslint && yarn run jscs && yarn run flow",
    "lint:fix": "node_package/scripts/lint-fix",
    "check": "yarn run lint && yarn run test",

where is flow?

---

docs/additional-reading/node-dependencies-and-npm.md, line 9 at r1 (raw file):

npm install -g npm-check-updates
rm npm-shrinkwrap.json
npm-check-updates -u

this updates the dependencies

I still use this with yarn

Please investigate

---

lib/generators/react_on_rails/dev_tests_generator.rb, line 33 at r1 (raw file):

        old_contents = File.read(package_json)
        new_contents = old_contents.gsub(/"react-on-rails": ".+",/,
                                         '"react-on-rails": "file:../../../..",')

why this change?

yarn change?

---

lib/react_on_rails/test_helper/node_process_launcher.rb, line 4 at r1 (raw file):

  module TestHelper
    def self.launch_node
      if ReactOnRails.configuration.server_render_method == "NodeJS"

this will probably cause a lint error

rubocop wants a guard clause

---

spec/dummy/package.json, line 45 at r1 (raw file):

    "react": ">= 0.14",
    "react-dom": ">= 0.14",
    "react-on-rails": "file:../.."

this is wrong

all of this should be in spec/dummy/client/package.json

---

spec/react_on_rails/generators/dev_tests_generator_spec.rb, line 27 at r1 (raw file):

    it "changes package.json to use local react-on-rails version of module" do
      assert_file("client/package.json") do |contents|
        assert_match('"react-on-rails": "file:../../../.."', contents)

Not sure on this one. Please explain.

---

spec/react_on_rails/generators/install_generator_spec.rb, line 94 at r1 (raw file):

              bundle && yarn

          - Run the foreman command to load the rails server and webpack process

start?

why change to load?

---

Comments from Reviewable

[justin808]

@squadette I'm going to wait to hear from you. I don't understand why you moved node_package. It doesn't make sense for react_on_rails to have a subdirectory called react-on-rails.

---

Reviewed 68 of 68 files at r2.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

.gitignore, line 25 at r2 (raw file):

node_modules

react-on-rails/node_package/lib

why this change?

---

package.json, line 59 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

where is flow?

@squadette Please try to answer my questions within https://reviewable.io/reviews/shakacode/react_on_rails/715#-KcntjgzwxQ2q74Nj54p

---

Comments from Reviewable

[squadette]

I don't understand why you moved node_package.

Of course. I had a theory, but looks like i"m wrong. I'll either move it back or explain why this change is needed. I'm still fighting :)

[squadette]

Okay, looks like tests pass: https://travis-ci.org/squadette/react_on_rails/builds/200978571

Except for eslint warnings in: https://travis-ci.org/squadette/react_on_rails/builds/200978571#L4634

[squadette]

Review status: 43 of 54 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

.travis.yml, line 9 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

should not be removed

we want to support older versions of ruby and also ruby 2.4!

Of course, it's just for my tests to be faster. Before the merge this part will be reverted. Thanks for the reminder.

---

spec/dummy/package.json, line 45 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

this is wrong

all of this should be in spec/dummy/client/package.json

you're right, I remove this in a separate commit. Thank you,

---

Comments from Reviewable

[justin808]

@squadette Let me know when you're ready for more review.

[coveralls]

Coverage remained the same at 99.329% when pulling 74e228a on squadette:yarn-for-upstream into 5d42157 on shakacode:master.

[coveralls]

Coverage remained the same at 99.329% when pulling 9f2e5ab on squadette:yarn-for-upstream into 5d42157 on shakacode:master.

[coveralls]

Coverage remained the same at 99.329% when pulling 7f5239d on squadette:yarn-for-upstream into 5d42157 on shakacode:master.

[coveralls]

Coverage remained the same at 99.329% when pulling 7f5239d on squadette:yarn-for-upstream into 5d42157 on shakacode:master.

[coveralls]

Coverage remained the same at 99.329% when pulling 0e67ec2 on squadette:yarn-for-upstream into 5d42157 on shakacode:master.

[coveralls]

Coverage remained the same at 99.329% when pulling 90c6f6e on squadette:yarn-for-upstream into 5d42157 on shakacode:master.

[coveralls]

Coverage remained the same at 99.329% when pulling 58e22b6 on squadette:yarn-for-upstream into 5d42157 on shakacode:master.

[squadette]

Review status: 41 of 57 files reviewed at latest revision, 10 unresolved discussions, some commit checks broke.

---

.gitignore, line 25 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

why this change?

I think I've seen yarn creating those files, but maybe it's a false memory.

---

.travis.yml, line 9 at r1 (raw file):

Previously, squadette (Alexey Mahotkin) wrote…

Of course, it's just for my tests to be faster. Before the merge this part will be reverted. Thanks for the reminder.

returned all the versions. Tries to introduce 2.4.0, but that's too hard for now, there will be a separate PR.

---

CONTRIBUTING.md, line 99 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Please double check on if there is a yarn link that we can use.

Let's try to change to yarn link later. I don't understand exactly how yarn link works.

---

package.json, line 59 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@squadette Please try to answer my questions within https://reviewable.io/reviews/shakacode/react_on_rails/715#-KcntjgzwxQ2q74Nj54p

Merge issue. Thank you,

---

docs/additional-reading/node-dependencies-and-npm.md, line 9 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

this updates the dependencies

I still use this with yarn

Please investigate

To upgrade all dependencies, use yarn upgrade.

---

lib/react_on_rails/test_helper/node_process_launcher.rb, line 4 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

this will probably cause a lint error

rubocop wants a guard clause

yep, fixed.

---

spec/dummy/package.json, line 45 at r1 (raw file):

Previously, squadette (Alexey Mahotkin) wrote…

you're right, I remove this in a separate commit. Thank you,

fixed

---

spec/react_on_rails/generators/dev_tests_generator_spec.rb, line 27 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Not sure on this one. Please explain.

It just checks for a string, and the string changed because of yarn syntax for modules in a directory.

---

spec/react_on_rails/generators/install_generator_spec.rb, line 94 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

start?

why change to load?

This is taken from your commit cfc10a9#diff-08931bab4b6ac3bc6fa0986cadb6a576 :) If you don't like the wording then please change it as you wish.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 99.329% when pulling 5301e81 on squadette:yarn-for-upstream into 932b462 on shakacode:master.

[squadette]

Update: Travis passed: https://travis-ci.org/squadette/react_on_rails/builds/202086175

@justin808, please consider merging this PR.

Thank you,

[squadette]

Review status: 42 of 54 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

lib/generators/react_on_rails/dev_tests_generator.rb, line 33 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

why this change?

yarn change?

yes. a syntax for module in a directory.

---

Comments from Reviewable

[justin808]

A few comments. Getting close!

CC: @robwise @alexfedoseev

---

Reviewed 14 of 66 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

.travis.yml, line 41 at r3 (raw file):

  - nvm install node
  - node -v
  - npm i -g yarn@0.19.0

0.19.1 but check

maybe we best not specify the version and we'll get the latest

---

package.json, line 86 at r3 (raw file):

  "homepage": "https://github.com/shakacode/react_on_rails#readme",
  "dependencies": {
    "jscs": "^3.0.7"

what is this? this probably needs to be removed.

eslint replaced jscs a while back.

---

spec/dummy/client/package.json, line 66 at r3 (raw file):

  },
  "scripts": {
    "lint": "yarn run eslint --silent && yarn run jscs --silent",

remove jscs

---

spec/dummy/client/package.json, line 69 at r3 (raw file):

    "eslint": "eslint --ext .js,.jsx .",
    "jscs": "jscs --verbose .",
    "jscs:fix": "jscs --verbose -x . --silent",

get rid of jscs

---

spec/react_on_rails/generators/install_generator_spec.rb, line 94 at r1 (raw file):

Run the foreman command to start the rails server and run webpack in watch mode.

let's revert that line

---

Comments from Reviewable

[squadette]

Review status: 48 of 54 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

.travis.yml, line 41 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

0.19.1 but check

maybe we best not specify the version and we'll get the latest

0.20.3 even. (no we did not get the latest last time I tried. Plus it's better to control the testing environment.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 99.329% when pulling 25e9a00 on squadette:yarn-for-upstream into 932b462 on shakacode:master.

[justin808]

Thanks for the contribution! Great work!

---

Reviewed 8 of 8 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 99.329% when pulling bdf9e81 on squadette:yarn-for-upstream into 932b462 on shakacode:master.

[coveralls]

Coverage remained the same at 99.329% when pulling bdf9e81 on squadette:yarn-for-upstream into 932b462 on shakacode:master.

[coveralls]

Coverage remained the same at 99.329% when pulling bdf9e81 on squadette:yarn-for-upstream into 932b462 on shakacode:master.

[squadette]

Updated finally. My build passes: https://travis-ci.org/squadette/react_on_rails/builds/202424502

shakacode build could just be restarted, it's a spurious error.

Thanks,

[squadette]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

package.json, line 86 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

what is this? this probably needs to be removed.

eslint replaced jscs a while back.

Done.

---

spec/dummy/client/package.json, line 66 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

remove jscs

Done.

---

spec/dummy/client/package.json, line 69 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

get rid of jscs

Done.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 99.329% when pulling 84febce on squadette:yarn-for-upstream into 610d8f0 on shakacode:master.

[coveralls]

Coverage remained the same at 99.329% when pulling 84febce on squadette:yarn-for-upstream into 610d8f0 on shakacode:master.

[justin808]

---

Reviewed 3 of 3 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[justin808]

@squadette I'm testing locally and I'll merge if I see no issues!

[justin808]

Great job @squadette! Next is Webpack v2.2.0 and the improved serialization of props!

[squadette]

Thank for merging :)

[squadette]

Btw, I've just migrated my project to Webpack 2.2.0. It required two trivial changes to webpack.config.js:

diff --git a/client/webpack.config.js b/client/webpack.config.js
index f9a1c6c..7ac7951 100644
--- a/client/webpack.config.js
+++ b/client/webpack.config.js
@@ -27,7 +27,7 @@ const config = {
   },
 
   resolve: {
-    extensions: ['', '.js', '.jsx'],
+    extensions: ['.js', '.jsx'],
     alias: {
       react: path.resolve('./node_modules/react'),
       'react-dom': path.resolve('./node_modules/react-dom'),
@@ -44,7 +44,7 @@ const config = {
     loaders: [
       {
         test: require.resolve('react'),
-        loader: 'imports?shim=es5-shim/es5-shim&sham=es5-shim/es5-sham',
+        loader: 'imports-loader?shim=es5-shim/es5-shim&sham=es5-shim/es5-sham',
       },
       {
         test: /\.jsx?$/,

Also, dedupePlugin needs to be removed, and tree shaking can enabled by

diff --git a/client/.babelrc b/client/.babelrc
index 9b7d435..0b1ef27 100644
--- a/client/.babelrc
+++ b/client/.babelrc
@@ -1,3 +1,3 @@
 {
-  "presets": ["es2015", "stage-0", "react"]
+  "presets": [ ["es2015", { modules: false } ], "stage-0", "react"]
 }

After that, everything works at least for me.

[justin808]

@squadette Yes, let's convert the whole project to be using Webpack 2.2.x!