diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c0420364..c73b43926 100644 --- 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 index 469344b15..e231638ed 100644 --- 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 index 13aab0d9f..1d486f81b 100644 --- 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 index d39329784..b4595cdd4 100644 --- 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 index 0c124e6a2..592ef6044 100644 --- 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 index 0a17948e7..09a31ffa1 100644 --- a/node_package/src/clientStartup.js +++ b/node_package/src/clientStartup.js @@ -90,6 +90,10 @@ 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. @@ -97,11 +101,11 @@ DELEGATING TO RENDERER ${name} for dom node with id: ${domNodeId} with props, ra */ 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 index 32a706813..8574f73e5 100644 --- a/spec/dummy/client/yarn.lock +++ b/spec/dummy/client/yarn.lock @@ -4146,7 +4146,7 @@ react-dom@^15.5.4: object-assign "^4.1.0" prop-types "~15.5.7" -react-helmet@5.0.3: +react-helmet@^5.0.3: version "5.0.3" resolved "https://registry.yarnpkg.com/react-helmet/-/react-helmet-5.0.3.tgz#c6da63ee96e83aa7c8fe6d041f28dd288b1b006d" dependencies: @@ -4156,7 +4156,7 @@ react-helmet@5.0.3: react-side-effect "^1.1.0" "react-on-rails@file:../../..": - version "6.9.3" + 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 index de94c8034..761752552 100644 --- 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\":\"\"}" 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 " 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,15 +76,17 @@ let(:id) { "App-react-component-0" } let(:react_definition_script) do - '" + <<-SCRIPT + + SCRIPT end let(:react_definition_script_no_params) do - '" + <<-SCRIPT + + SCRIPT end context "with json string props" do @@ -92,13 +94,13 @@ "{\"hello\":\"world\",\"free\":\"of charge\",\"x\":\"\"}" 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 + + 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 index 063717363..8757c532c 100644 --- 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 diff --git a/spec/react_on_rails/version_checker_spec.rb b/spec/react_on_rails/version_checker_spec.rb index 7cda2b681..d3a62c8c8 100644 --- a/spec/react_on_rails/version_checker_spec.rb +++ b/spec/react_on_rails/version_checker_spec.rb @@ -14,69 +14,75 @@ module ReactOnRails describe "#warn_if_gem_and_node_package_versions_differ" do let(:logger) { FakeLogger.new } - context "when gem and node package major versions are equal" do - let(:node_package_version) { double_package_version(raw: "^2.2.5", major: "2") } - before { stub_gem_version("2.0.0.beta.2") } + context "when gem and node package major and minor versions are equal" do + let(:node_package_version) do + double_package_version(raw: "^2.2.5-beta.2", major_minor_patch: %w(2 2 5)) + end + before { stub_gem_version("2.2.5.beta.2") } - it "does not log a warning" do - check_version(node_package_version, logger) - expect(logger.message).to be_nil + it "does not raise" do + expect { check_version(node_package_version) }.not_to raise_error end end context "when gem and node package major versions differ" do let(:node_package_version) do - double_package_version(raw: "13.0.0.beta-2", major: "13") + double_package_version(raw: "13.0.0.beta-2", major_minor_patch: %w(13 0 0)) end before { stub_gem_version("12.0.0.beta.1") } - it "logs a warning" do - check_version(node_package_version, logger) - expect(logger.message).to be_present + it "raises" do + error = /ReactOnRails: ReactOnRails gem and node package versions do not match/ + expect { check_version(node_package_version) }.to raise_error(error) end end - context "when package json uses a relative path with dots" do + context "when gem and node package major versions match and minor differs" do let(:node_package_version) do - double_package_version(raw: "../../..", major: "", relative_path: true) + double_package_version(raw: "13.0.0.beta-2", major_minor_patch: %w(13 0 0)) end - before { stub_gem_version("2.0.0.beta.1") } + before { stub_gem_version("13.1.0") } - it "does not log a warning" do - check_version(node_package_version, logger) - expect(logger.message).to be_nil + it "raises" do + error = /ReactOnRails: ReactOnRails gem and node package versions do not match/ + expect { check_version(node_package_version) }.to raise_error(error) end end - context "when package json uses a one-digit version string" do + context "when gem and node package major, minor versions match and patch differs" do let(:node_package_version) do - double_package_version(raw: "^6", major: "6") + double_package_version(raw: "13.0.1", major_minor_patch: %w(13 0 1)) end + before { stub_gem_version("13.0.0") } - it "does not log a warning" do - stub_gem_version("6") - check_version(node_package_version, logger) - expect(logger.message).to be_nil + it "raises" do + error = /ReactOnRails: ReactOnRails gem and node package versions do not match/ + expect { check_version(node_package_version) }.to raise_error(error) end + end - it "logs a warning" do - stub_gem_version("5") - check_version(node_package_version, logger) - expect(logger.message).to be_present + context "when package json uses a relative path with dots" do + let(:node_package_version) do + double_package_version(raw: "../../..", major_minor_patch: "", relative_path: true) + end + before { stub_gem_version("2.0.0.beta.1") } + + it "does not raise" do + expect { check_version(node_package_version) }.not_to raise_error end end end - def double_package_version(raw: nil, major: nil, relative_path: false) + def double_package_version(raw: nil, major_minor_patch: nil, relative_path: false) instance_double(VersionChecker::NodePackageVersion, raw: raw, - major: major, + major_minor_patch: major_minor_patch, relative_path?: relative_path) end - def check_version(node_package_version, logger) - version_checker = VersionChecker.new(node_package_version, logger) - version_checker.warn_if_gem_and_node_package_versions_differ + def check_version(node_package_version) + version_checker = VersionChecker.new(node_package_version) + version_checker.raise_if_gem_and_node_package_versions_differ end describe VersionChecker::NodePackageVersion do @@ -94,7 +100,7 @@ def check_version(node_package_version, logger) end describe "#major" do - specify { expect(node_package_version.major).to eq("0") } + specify { expect(node_package_version.major_minor_patch).to eq(%w(0 0 2)) } end end @@ -109,8 +115,8 @@ def check_version(node_package_version, logger) specify { expect(node_package_version.relative_path?).to be false } end - describe "#major" do - specify { expect(node_package_version.major).to eq("14") } + describe "#major_minor_patch" do + specify { expect(node_package_version.major_minor_patch).to eq(%w(14 0 0)) } end end @@ -126,7 +132,7 @@ def check_version(node_package_version, logger) end describe "#major" do - specify { expect(node_package_version.major).to be_nil } + specify { expect(node_package_version.major_minor_patch).to be_nil } end end @@ -142,7 +148,7 @@ def check_version(node_package_version, logger) end describe "#major" do - specify { expect(node_package_version.major).to be_nil } + specify { expect(node_package_version.major_minor_patch).to be_nil } end end end