Performance and throw if version mismatch (#821)

* Any version mismatch of gem and node package throws an error. * 6.9 introduced code that would parse the props into a Hash unnecessarily. Any customers with significant String props took a performance hit from this on page rendering. * Fix is to only put props inside of the script tag, and not any meta data, like the dom id and name of the component. This avoids unnecessary parsing.
shakacode · Apr 26, 2017 · 36b5caa · 36b5caa
1 parent b0ec86a
commit 36b5caa
Show file tree

Hide file tree

Showing 10 changed files with 126 additions and 116 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,9 +1,16 @@
 # Change Log
-All notable changes to this project's source code will be documented in this file. Items under `Unreleased` is upcoming features that will be out in next version.
+All notable changes to this project's source code will be documented in this file. Items under `Unreleased` is upcoming features that will be out in next version. NOTE: major versions of the npm module and the gem must be kept in sync.
 
 Contributors: please follow the recommendations outlined at [keepachangelog.com](http://keepachangelog.com/). Please use the existing headings and styling as a guide, and add a link for the version diff at the bottom of the file. Also, please update the `Unreleased` link to compare to the latest release version.
 
 ## [Unreleased]
+## [7.0.0] - 2017-04-25
+### Changed
+- Any version differences in gem and node package for React on Rails throw an error [#821](https://github.com/shakacode/react_on_rails/pull/821) by [justin808](https://github.com/justin808)
+
+### Fixed
+- Fixes serious performance regression when using String props for rendering. [#821](https://github.com/shakacode/react_on_rails/pull/821) by [justin808](https://github.com/justin808)
+
 ## [6.10.1] - 2017-04-23
 ### Fixed
 - Improve json conversion with tests and support for older Rails 3.x. [#787](https://github.com/shakacode/react_on_rails/pull/787) by [cheremukhin23](https://github.com/cheremukhin23) and [Ynote](https://github.com/Ynote).
@@ -534,7 +541,8 @@ Best done with Object destructing:
 ##### Fixed
 - Fix several generator related issues.
 
-[Unreleased]: https://github.com/shakacode/react_on_rails/compare/6.10.1...master
+[Unreleased]: https://github.com/shakacode/react_on_rails/compare/7.0.0...master
+[7.0.0]: https://github.com/shakacode/react_on_rails/compare/6.10.1...7.0.0
 [6.10.1]: https://github.com/shakacode/react_on_rails/compare/6.10.0...6.10.1
 [6.10.0]: https://github.com/shakacode/react_on_rails/compare/6.9.3...6.10.0
 [6.9.3]: https://github.com/shakacode/react_on_rails/compare/6.9.1...6.9.3

diff --git a/app/helpers/react_on_rails_helper.rb b/app/helpers/react_on_rails_helper.rb
@@ -107,12 +107,17 @@ def react_component(component_name, raw_options = {})
     # Setup the page_loaded_js, which is the same regardless of prerendering or not!
     # The reason is that React is smart about not doing extra work if the server rendering did its job.
     component_specification_tag = content_tag(:script,
-                                              json_safe_and_pretty(options.data).html_safe,
+                                              json_safe_and_pretty(options.props).html_safe,
                                               type: "application/json",
-                                              class: "js-react-on-rails-component")
+                                              class: "js-react-on-rails-component",
+                                              "data-component-name" => options.name,
+                                              "data-trace" => options.trace,
+                                              "data-dom-id" => options.dom_id)
 
     # Create the HTML rendering part
-    result = server_rendered_react_component_html(options.props, options.name, options.dom_id,
+    result = server_rendered_react_component_html(options.props,
+                                                  options.name,
+                                                  options.dom_id,
                                                   prerender: options.prerender,
                                                   trace: options.trace,
                                                   raise_on_prerender_error: options.raise_on_prerender_error)

diff --git a/lib/react_on_rails/engine.rb b/lib/react_on_rails/engine.rb
@@ -2,7 +2,7 @@ module ReactOnRails
   class Engine < ::Rails::Engine
     config.to_prepare do
       if File.exist?(VersionChecker::NodePackageVersion.package_json_path)
-        VersionChecker.build.warn_if_gem_and_node_package_versions_differ
+        VersionChecker.build.raise_if_gem_and_node_package_versions_differ
       end
       ReactOnRails::ServerRenderingPool.reset_pool
     end

diff --git a/lib/react_on_rails/react_component/options.rb b/lib/react_on_rails/react_component/options.rb
@@ -13,8 +13,7 @@ def initialize(name: required("name"), options: required("options"))
       end
 
       def props
-        props = options.fetch(:props) { NO_PROPS }
-        props.is_a?(String) ? JSON.parse(ERB::Util.json_escape(props)) : props
+        options.fetch(:props) { NO_PROPS }
       end
 
       def name
@@ -45,15 +44,6 @@ def raise_on_prerender_error
         retrieve_key(:raise_on_prerender_error)
       end
 
-      def data
-        {
-          component_name: name,
-          props: props,
-          trace: trace,
-          dom_id: dom_id
-        }
-      end
-
       private
 
       attr_reader :options

diff --git a/lib/react_on_rails/version_checker.rb b/lib/react_on_rails/version_checker.rb
@@ -2,44 +2,49 @@ module ReactOnRails
   # Responsible for checking versions of rubygem versus yarn node package
   # against each otherat runtime.
   class VersionChecker
-    attr_reader :node_package_version, :logger
-    MAJOR_VERSION_REGEX = /(\d+)\.?/
+    attr_reader :node_package_version
+    MAJOR_MINOR_PATCH_VERSION_REGEX = /(\d+)\.(\d+)\.(\d+)/
 
     def self.build
-      new(NodePackageVersion.build, Rails.logger)
+      new(NodePackageVersion.build)
     end
 
-    def initialize(node_package_version, logger)
-      @logger = logger
+    def initialize(node_package_version)
       @node_package_version = node_package_version
     end
 
     # 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 warn_if_gem_and_node_package_versions_differ
+    def raise_if_gem_and_node_package_versions_differ
       return if node_package_version.relative_path?
-      return if node_package_version.major == gem_major_version
-      log_differing_versions_warning
+      node_major_minor_patch = node_package_version.major_minor_patch
+      gem_major_minor_patch = gem_major_minor_patch_version
+      return if 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
     end
 
     private
 
-    def log_differing_versions_warning
-      msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package MAJOR versions do not match\n" \
+    def raise_differing_versions_warning
+      msg = "**ERROR** ReactOnRails: ReactOnRails gem and node package versions do not match\n" \
             "                     gem: #{gem_version}\n" \
             "            node package: #{node_package_version.raw}\n" \
-            "Ensure the installed MAJOR version of the gem is the same as the MAJOR version of \n"\
+            "Ensure the installed version of the gem is the same as the version of \n"\
             "your installed node package."
-      logger.error(msg)
+      raise msg
     end
 
     def gem_version
       ReactOnRails::VERSION
     end
 
-    def gem_major_version
-      gem_version.match(MAJOR_VERSION_REGEX)[1]
+    def gem_major_minor_patch_version
+      match = gem_version.match(MAJOR_MINOR_PATCH_VERSION_REGEX)
+      [match[1], match[2], match[3]]
     end
 
     class NodePackageVersion
@@ -71,9 +76,10 @@ def relative_path?
         raw.match(%r{(\.\.|\Afile:///)}).present?
       end
 
-      def major
+      def major_minor_patch
         return if relative_path?
-        raw.match(MAJOR_VERSION_REGEX)[1]
+        match = raw.match(MAJOR_MINOR_PATCH_VERSION_REGEX)
+        [match[1], match[2], match[3]]
       end
 
       private

diff --git a/node_package/src/clientStartup.js b/node_package/src/clientStartup.js
@@ -90,18 +90,22 @@ DELEGATING TO RENDERER ${name} for dom node with id: ${domNodeId} with props, ra
   return false;
 }
 
+function domNodeIdForEl(el) {
+  return el.getAttribute('data-dom-id');
+}
+
 /**
  * Used for client rendering by ReactOnRails. Either calls ReactDOM.render or delegates
  * to a renderer registered by the user.
  * @param el
  */
 function render(el, railsContext) {
   const context = findContext();
-  const jsonEl = JSON.parse(el.textContent);
-  const name = jsonEl.component_name;
-  const domNodeId = jsonEl.dom_id;
-  const props = jsonEl.props;
-  const trace = jsonEl.trace;
+  // This must match app/helpers/react_on_rails_helper.rb:113
+  const name = el.getAttribute('data-component-name');
+  const domNodeId = domNodeIdForEl(el);
+  const props = JSON.parse(el.textContent);
+  const trace = el.getAttribute('data-trace');
 
   try {
     const domNode = document.getElementById(domNodeId);
@@ -152,10 +156,14 @@ export function reactOnRailsPageLoaded() {
 }
 
 function unmount(el) {
-  const elData = JSON.parse(el.textContent);
-  const domNodeId = elData.dom_id;
+  const domNodeId = domNodeIdForEl(el);
   const domNode = document.getElementById(domNodeId);
-  ReactDOM.unmountComponentAtNode(domNode);
+  try {
+    ReactDOM.unmountComponentAtNode(domNode);
+  } catch (e) {
+    console.info(`Caught error calling unmountComponentAtNode: ${e.message} for domNode`,
+      domNode, e);
+  }
 }
 
 function reactOnRailsPageUnloaded() {

diff --git a/spec/dummy/client/yarn.lock b/spec/dummy/client/yarn.lock
@@ -4177,8 +4177,8 @@ react-helmet@^5.0.3:
     prop-types "^15.5.4"
     react-side-effect "^1.1.0"
 
-"react-on-rails@file:../../../":
-  version "6.10.0"
+"react-on-rails@file:../../..":
+  version "6.10.1"
 
 react-proptypes@^0.0.1:
   version "0.0.1"

diff --git a/spec/dummy/spec/helpers/react_on_rails_helper_spec.rb b/spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
@@ -18,12 +18,12 @@
     }
   end
 
-  let(:hash_sanitized) do
+  let(:json_string_sanitized) do
     '{"hello":"world","free":"of charge","x":"\\u003c/script\\u003e\\u003cscrip'\
       "t\\u003ealert('foo')\\u003c/script\\u003e\"}"
   end
 
-  let(:hash_unsanitized) do
+  let(:json_string_unsanitized) do
     "{\"hello\":\"world\",\"free\":\"of charge\",\"x\":\"</script><script>alert('foo')</script>\"}"
   end
 
@@ -34,29 +34,29 @@
 
     it "converts a hash to escaped JSON" do
       escaped_json = helper.json_safe_and_pretty(hash)
-      expect(escaped_json).to eq(hash_sanitized)
+      expect(escaped_json).to eq(json_string_sanitized)
     end
 
     it "converts a string to escaped JSON" do
-      escaped_json = helper.json_safe_and_pretty(hash_unsanitized)
-      expect(escaped_json).to eq(hash_sanitized)
+      escaped_json = helper.json_safe_and_pretty(json_string_unsanitized)
+      expect(escaped_json).to eq(json_string_sanitized)
     end
   end
 
   describe "#sanitized_props_string(props)" do
     it "converts a hash to JSON and escapes </script>" do
       sanitized = helper.sanitized_props_string(hash)
-      expect(sanitized).to eq(hash_sanitized)
+      expect(sanitized).to eq(json_string_sanitized)
     end
 
     it "leaves a string alone that does not contain xss tags" do
-      sanitized = helper.sanitized_props_string(hash_sanitized)
-      expect(sanitized).to eq(hash_sanitized)
+      sanitized = helper.sanitized_props_string(json_string_sanitized)
+      expect(sanitized).to eq(json_string_sanitized)
     end
 
     it "fixes a string alone that contain xss tags" do
-      sanitized = helper.sanitized_props_string(hash_unsanitized)
-      expect(sanitized).to eq(hash_sanitized)
+      sanitized = helper.sanitized_props_string(json_string_unsanitized)
+      expect(sanitized).to eq(json_string_sanitized)
     end
   end
 
@@ -76,29 +76,31 @@
     let(:id) { "App-react-component-0" }
 
     let(:react_definition_script) do
-      '<script type="application/json" class="js-react-on-rails-component">'\
-        '{"component_name":"App","props":{"name":"My Test Name"},"trace":false,"dom_id":"App-react-component-0"}'\
-      "</script>"
+      <<-SCRIPT
+<script type="application/json" class="js-react-on-rails-component" data-component-name="App" \
+data-trace="false" data-dom-id="App-react-component-0">{"name":"My Test Name"}</script>
+      SCRIPT
     end
 
     let(:react_definition_script_no_params) do
-      '<script type="application/json" class="js-react-on-rails-component">'\
-        '{"component_name":"App","props":{},"trace":false,"dom_id":"App-react-component-0"}'\
-      "</script>"
+      <<-SCRIPT
+<script type="application/json" class="js-react-on-rails-component" data-component-name="App" \
+data-trace="false" data-dom-id="App-react-component-0">{}</script>
+      SCRIPT
     end
 
     context "with json string props" do
       let(:json_props) do
         "{\"hello\":\"world\",\"free\":\"of charge\",\"x\":\"</script><script>alert('foo')</script>\"}"
       end
 
-      let(:props_sanitized) do
+      let(:json_props_sanitized) do
         '{"hello":"world","free":"of charge","x":"\\u003c/script\\u003e\\u003cscrip'\
           "t\\u003ealert('foo')\\u003c/script\\u003e\"}"
       end
 
       subject { react_component("App", props: json_props) }
-      it { is_expected.to include props_sanitized }
+      it { is_expected.to include json_props_sanitized }
     end
 
     describe "API with component name only" do
@@ -122,9 +124,10 @@
       let(:id) { "shaka_div" }
 
       let(:react_definition_script) do
-        '<script type="application/json" class="js-react-on-rails-component">'\
-          '{"component_name":"App","props":{"name":"My Test Name"},"trace":false,"dom_id":"shaka_div"}'\
-        "</script>"
+        <<-SCRIPT
+<script type="application/json" class="js-react-on-rails-component" data-component-name="App" \
+data-trace="false" data-dom-id="shaka_div">{"name":"My Test Name"}</script>
+        SCRIPT
       end
 
       it { is_expected.to include id }

diff --git a/spec/react_on_rails/react_component/options_spec.rb b/spec/react_on_rails/react_component/options_spec.rb
@@ -110,22 +110,6 @@ def the_attrs(name: "App", options: {})
     end
   end
 
-  describe "#data" do
-    it "returns data for component" do
-      attrs = the_attrs(name: "app", options: { trace: false, id: 2 })
-      expected_data = {
-        component_name: "App",
-        props: {},
-        trace: false,
-        dom_id: 2
-      }
-
-      opts = described_class.new(attrs)
-
-      expect(opts.data).to eq expected_data
-    end
-  end
-
   CONFIGURABLE_OPTIONS.each do |option|
     describe "##{option}" do
       context "with #{option} option" do