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

@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):

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

Comments from Reviewable

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 :)

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

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):

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):

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

Comments from Reviewable

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

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

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

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

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

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

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

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

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

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

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

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

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):

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):

Merge issue. Thank you,

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

To upgrade all dependencies, use yarn upgrade.

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

yep, fixed.

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

fixed

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

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):

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

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

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

@justin808, please consider merging this PR.

Thank you,

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):

yes. a syntax for module in a directory.

Comments from Reviewable

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 [email protected]

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

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):

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

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

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

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.

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