Fix Version Checker's hard-coded path for package.json (#1657)

* check both root & client packages
* add logging methods instead of changing raise methods

Author: Judahmeek

CHANGELOG.md

@@ -18,7 +18,12 @@ Please follow the recommendations outlined at [keepachangelog.com](http://keepac
 ### [Unreleased]
 Changes since the last non-beta release.
 
-#### Added(https://github.com/AbanoubGhadban).
+#### Fixed
+
+- Changed the ReactOnRails' version checker to use `ReactOnRails.configuration.node_modules_location` to determine the location of the package.json that the `react-on-rails` dependency is expected to be set by.
+- Also, all errors that would be raised by the version checking have been converted to `Rails.Logger` warnings to avoid any breaking changes. [PR 1657](https://github.com/shakacode/react_on_rails/pull/1657) by [judahmeek](https://github.com/judahmeek).
+
+#### Added
 - Added streaming server rendering support:
   - [PR #1633](https://github.com/shakacode/react_on_rails/pull/1633) by [AbanoubGhadban](https://github.com/AbanoubGhadban).
     - New `stream_react_component` helper for adding streamed components to views

lib/react_on_rails/engine.rb

@@ -5,9 +5,7 @@
 module ReactOnRails
   class Engine < ::Rails::Engine
     config.to_prepare do
-      if File.exist?(VersionChecker::NodePackageVersion.package_json_path)
-        VersionChecker.build.raise_if_gem_and_node_package_versions_differ
-      end
+      VersionChecker.build.log_if_gem_and_node_package_versions_differ
       ReactOnRails::ServerRenderingPool.reset_pool
     end
   end

lib/react_on_rails/version_checker.rb

@@ -19,18 +19,17 @@ def initialize(node_package_version)
     # For compatibility, the gem and the node package versions should always match,
     # unless the user really knows what they're doing. So we will give a
     # warning if they do not.
-    def raise_if_gem_and_node_package_versions_differ
-      return if node_package_version.relative_path?
+    def log_if_gem_and_node_package_versions_differ
+      return if node_package_version.raw.nil? || node_package_version.relative_path?
+      return log_node_semver_version_warning if node_package_version.semver_wildcard?
 
       node_major_minor_patch = node_package_version.major_minor_patch
       gem_major_minor_patch = gem_major_minor_patch_version
       versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] &&
                        node_major_minor_patch[1] == gem_major_minor_patch[1] &&
                        node_major_minor_patch[2] == gem_major_minor_patch[2]
 
-      raise_differing_versions_warning unless versions_match
-
-      raise_node_semver_version_warning if node_package_version.semver_wildcard?
+      log_differing_versions_warning unless versions_match
     end
 
     private
@@ -46,15 +45,15 @@ def common_error_msg
       MSG
     end
 
-    def raise_differing_versions_warning
-      msg = "**ERROR** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}"
-      raise ReactOnRails::Error, msg
+    def log_differing_versions_warning
+      msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}"
+      Rails.logger.warn(msg)
     end
 
-    def raise_node_semver_version_warning
-      msg = "**ERROR** ReactOnRails: Your node package version for react-on-rails contains a " \
+    def log_node_semver_version_warning
+      msg = "**WARNING** ReactOnRails: Your node package version for react-on-rails contains a " \
             "^ or ~\n#{common_error_msg}"
-      raise ReactOnRails::Error, msg
+      Rails.logger.warn(msg)
     end
 
     def gem_version
@@ -74,29 +73,33 @@ def self.build
       end
 
       def self.package_json_path
-        Rails.root.join("client", "package.json")
+        Rails.root.join(ReactOnRails.configuration.node_modules_location, "package.json")
       end
 
       def initialize(package_json)
         @package_json = package_json
       end
 
       def raw
-        parsed_package_contents = JSON.parse(package_json_contents)
-        if parsed_package_contents.key?("dependencies") &&
-           parsed_package_contents["dependencies"].key?("react-on-rails")
-          parsed_package_contents["dependencies"]["react-on-rails"]
-        else
-          raise ReactOnRails::Error, "No 'react-on-rails' entry in package.json dependencies"
+        if File.exist?(package_json)
+          parsed_package_contents = JSON.parse(package_json_contents)
+          if parsed_package_contents.key?("dependencies") &&
+             parsed_package_contents["dependencies"].key?("react-on-rails")
+            return parsed_package_contents["dependencies"]["react-on-rails"]
+          end
         end
+        msg = "No 'react-on-rails' entry in the dependencies of #{NodePackageVersion.package_json_path}, " \
+              "which is the expected location according to ReactOnRails.configuration.node_modules_location"
+        Rails.logger.warn(msg)
+        nil
       end
 
       def semver_wildcard?
         raw.match(/[~^]/).present?
       end
 
       def relative_path?
-        raw.match(%r{(\.\.|\Afile:///)}).present?
+        raw.match(/(\.\.|\Afile:)/).present?
       end
 
       def major_minor_patch

spec/react_on_rails/fixtures/yalc_package.json

@@ -0,0 +1,10 @@
+{
+  "dependencies": {
+    "babel": "^6.3.26",
+    "react-on-rails": "file:.yalc/react-on-rails",
+    "webpack": "^1.12.8"
+  },
+  "devDependencies": {
+    "babel-eslint": "^5.0.0-beta6"
+  }
+}

spec/react_on_rails/version_checker_spec.rb

@@ -11,7 +11,7 @@ def error(message)
   end
 end
 
-module ReactOnRails
+module ReactOnRails # rubocop:disable Metrics/ModuleLength
   describe VersionChecker do
     describe "#warn_if_gem_and_node_package_versions_differ" do
       let(:logger) { FakeLogger.new }
@@ -23,8 +23,10 @@ module ReactOnRails
 
         before { stub_gem_version("2.2.5.beta.2") }
 
-        it "does not raise" do
-          expect { check_version(node_package_version) }.not_to raise_error
+        it "does not log" do
+          allow(Rails.logger).to receive(:warn)
+          check_version_and_log(node_package_version)
+          expect(Rails.logger).not_to have_received(:warn)
         end
       end
 
@@ -35,9 +37,11 @@ module ReactOnRails
 
         before { stub_gem_version("2.2.5") }
 
-        it "does raise" do
-          error = /ReactOnRails: Your node package version for react-on-rails contains a \^ or ~/
-          expect { check_version(node_package_version) }.to raise_error(error)
+        it "logs" do
+          allow(Rails.logger).to receive(:warn)
+          message = /ReactOnRails: Your node package version for react-on-rails contains a \^ or ~/
+          check_version_and_log(node_package_version)
+          expect(Rails.logger).to have_received(:warn).with(message)
         end
       end
 
@@ -48,9 +52,11 @@ module ReactOnRails
 
         before { stub_gem_version("12.0.0.beta.1") }
 
-        it "raises" do
-          error = /ReactOnRails: ReactOnRails gem and node package versions do not match/
-          expect { check_version(node_package_version) }.to raise_error(error)
+        it "logs" do
+          allow(Rails.logger).to receive(:warn)
+          message = /ReactOnRails: ReactOnRails gem and node package versions do not match/
+          check_version_and_log(node_package_version)
+          expect(Rails.logger).to have_received(:warn).with(message)
         end
       end
 
@@ -61,9 +67,11 @@ module ReactOnRails
 
         before { stub_gem_version("13.1.0") }
 
-        it "raises" do
-          error = /ReactOnRails: ReactOnRails gem and node package versions do not match/
-          expect { check_version(node_package_version) }.to raise_error(error)
+        it "logs" do
+          allow(Rails.logger).to receive(:warn)
+          message = /ReactOnRails: ReactOnRails gem and node package versions do not match/
+          check_version_and_log(node_package_version)
+          expect(Rails.logger).to have_received(:warn).with(message)
         end
       end
 
@@ -74,9 +82,11 @@ module ReactOnRails
 
         before { stub_gem_version("13.0.0") }
 
-        it "raises" do
-          error = /ReactOnRails: ReactOnRails gem and node package versions do not match/
-          expect { check_version(node_package_version) }.to raise_error(error)
+        it "logs" do
+          allow(Rails.logger).to receive(:warn)
+          message = /ReactOnRails: ReactOnRails gem and node package versions do not match/
+          check_version_and_log(node_package_version)
+          expect(Rails.logger).to have_received(:warn).with(message)
         end
       end
 
@@ -87,8 +97,20 @@ module ReactOnRails
 
         before { stub_gem_version("2.0.0.beta.1") }
 
-        it "does not raise" do
-          expect { check_version(node_package_version) }.not_to raise_error
+        it "does not log" do
+          allow(Rails.logger).to receive(:warn)
+          check_version_and_log(node_package_version)
+          expect(Rails.logger).not_to have_received(:warn)
+        end
+      end
+
+      context "when package json doesn't exist" do
+        let(:node_package_version) do
+          double_package_version(raw: nil)
+        end
+
+        it "log method returns nil" do
+          expect(check_version_and_log(node_package_version)).to be_nil
         end
       end
     end
@@ -102,14 +124,32 @@ def double_package_version(raw: nil, semver_wildcard: false,
                       relative_path?: relative_path)
     end
 
-    def check_version(node_package_version)
+    def check_version_and_raise(node_package_version)
       version_checker = VersionChecker.new(node_package_version)
       version_checker.raise_if_gem_and_node_package_versions_differ
     end
 
+    def check_version_and_log(node_package_version)
+      version_checker = VersionChecker.new(node_package_version)
+      version_checker.log_if_gem_and_node_package_versions_differ
+    end
+
     describe VersionChecker::NodePackageVersion do
       subject(:node_package_version) { described_class.new(package_json) }
 
+      describe "#build" do
+        it "initializes NodePackageVersion with ReactOnRails.configuration.node_modules_location" do
+          allow(ReactOnRails).to receive_message_chain(:configuration, :node_modules_location).and_return("spec/dummy")
+          root_package_json_path = File.expand_path("../../package.json", __dir__)
+          allow(Rails).to receive_message_chain(:root, :join).and_return(root_package_json_path)
+          message = "No 'react-on-rails' entry in the dependencies of #{root_package_json_path}, which is " \
+                    "the expected location according to ReactOnRails.configuration.node_modules_location"
+          allow(Rails.logger).to receive(:warn)
+          described_class.build.raw
+          expect(Rails.logger).to have_received(:warn).with(message)
+        end
+      end
+
       describe "#semver_wildcard?" do
         context "when package json lists an exact version of '0.0.2'" do
           let(:package_json) { File.expand_path("fixtures/normal_package.json", __dir__) }
@@ -193,6 +233,46 @@ def check_version(node_package_version)
           specify { expect(node_package_version.major_minor_patch).to be_nil }
         end
       end
+
+      context "with node version of 'file:.yalc/react-on-rails'" do
+        let(:package_json) { File.expand_path("fixtures/yalc_package.json", __dir__) }
+
+        describe "#raw" do
+          specify { expect(node_package_version.raw).to eq("file:.yalc/react-on-rails") }
+        end
+
+        describe "#relative_path?" do
+          specify { expect(node_package_version.relative_path?).to be true }
+        end
+
+        describe "#major" do
+          specify { expect(node_package_version.major_minor_patch).to be_nil }
+        end
+      end
+
+      context "with package.json without react-on-rails dependency" do
+        let(:package_json) { File.expand_path("../../package.json", __dir__) }
+
+        describe "#raw" do
+          it "returns nil" do
+            root_package_json_path = File.expand_path("fixtures/nonexistent_package.json", __dir__)
+            allow(Rails).to receive_message_chain(:root, :join).and_return(root_package_json_path)
+            expect(node_package_version.raw).to be_nil
+          end
+        end
+      end
+
+      context "with non-existing package.json" do
+        let(:package_json) { File.expand_path("fixtures/nonexistent_package.json", __dir__) }
+
+        describe "#raw" do
+          it "returns nil" do
+            root_package_json_path = File.expand_path("fixtures/nonexistent_package.json", __dir__)
+            allow(Rails).to receive_message_chain(:root, :join).and_return(root_package_json_path)
+            expect(node_package_version.raw).to be_nil
+          end
+        end
+      end
     end
   end
 end