Raise if versions differ and fix performance regression for string props #821

justin808 · 2017-04-24T23:32:00Z

Raise on Gem and Node Version Mismatch

Because many installations did not notice issues when versions differed, we're now going to force upgrading both the gem and node package at the same time.

Fix Performance Issue on String Props

6.9 introduced code that would parse the props into a Hash
unnecessarily. Any customers with significant String props took a huge
performance hit from this on page rendering.

Fix is to remove the unnecessary parsing.

I believe the parsing was done so that pretty printing of the JSON could
be done. If we introduce that feature in the future, it will:

Be configurable on whether or not to use it.
Possibly never run in production.

See #819.

CC: @cheremukhin23 @robwise @dylangrafmyre @squadette

Thanks @dzirtusss for finding this! AWESOME!!!

See 7ccd603#diff-ab00ff0a0648717a06431522f7eb6861R16

This change is

justin808 · 2017-04-25T10:22:31Z

Can I get a code review?

coveralls · 2017-04-25T10:39:18Z

Coverage decreased (-0.01%) to 98.02% when pulling 70ffd76 on fix-performance-regression-parsing-string-props into f65fd72 on master.

coveralls · 2017-04-25T10:50:57Z

Coverage decreased (-0.01%) to 98.02% when pulling 70ffd76 on fix-performance-regression-parsing-string-props into f65fd72 on master.

coveralls · 2017-04-25T10:50:57Z

Coverage decreased (-0.01%) to 98.02% when pulling 70ffd76 on fix-performance-regression-parsing-string-props into f65fd72 on master.

pjfitzgibbons · 2017-04-25T14:51:51Z

lib/react_on_rails/version_checker.rb

-    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" \


I'm confused? The PR/Issue and this text says "MAJOR must match..."... but the code above at ln:23 and tests below are requiring all 3 parts to be exact-match. If the logic above is correct, maybe some different wording here to indicate the versions must be exact-match?

Should I say "major, my minor, and patch versions"?

Ok, So I think I mis-read the diff at ln29:33, but there is no-change at ln: 32:36 that I think should change to read "Ensure the installed version of the gem is the same as the version of \n"\
At Changelog.md:2, maybe should say NOTE: exact versions of the npm module and the gem must be kept in sync.

kaizencodes · 2017-04-25T18:35:53Z

lib/react_on_rails/version_checker.rb

-    attr_reader :node_package_version, :logger
-    MAJOR_VERSION_REGEX = /(\d+)\.?/
+    attr_reader :node_package_version
+    MAJOR_MINOR_PATCH_VERSION_REGEX = /(\d+)\.(\d+)\.(\d+)/


A bit confusing name. VERSION_REGEX might be more readable.

kaizencodes · 2017-04-25T18:38:06Z

lib/react_on_rails/version_checker.rb

    end

    def gem_version
      ReactOnRails::VERSION
    end

-    def gem_major_version
-      gem_version.match(MAJOR_VERSION_REGEX)[1]
+    def gem_major_minor_patch_version


Same here, could be just: gem_version

kaizencodes · 2017-04-25T18:45:59Z

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.


Drop the major. NOTE: versions of the npm module and the gem must be kept in sync.

@kaizencodes has a great point. Since major-minor-patch-build/alpha/beta must match on both sides (?!), why not just direct-match the strings and be done with it? What effect will this have on "local-development" activity against ReactOnRails directly ? @justin808

If the desire is to match "only" M.M.P, then yes I agree rename the const VERSION_REGEX and also maybe pin the rx to begin-of-string VERSION_REGEX = /^(\d+)\.(\d+)\.(\d+)/

@pjfitzgibbons b/c of the way that beta versions in npm/rubygems use dash versus dots.

@pjfitzgibbons on the beginning of the string, you might have a '^'

* 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 huge 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. Fix is to remove the unnecessary parsing.

coveralls · 2017-04-25T22:08:23Z

Coverage decreased (-0.01%) to 98.02% when pulling ef40698 on fix-performance-regression-parsing-string-props into f65fd72 on master.

* 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.

mapreal19 · 2017-04-26T17:36:41Z

Reviewed 9 of 10 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.

lib/react_on_rails/version_checker.rb, line 6 at r1 (raw file):

Previously, kaizencodes (Daniel Szatmari) wrote…

A bit confusing name. VERSION_REGEX might be more readable.

agree

node_package/src/clientStartup.js, line 104 at r2 (raw file):

function render(el, railsContext) {
  const context = findContext();
  // This must match app/helpers/react_on_rails_helper.rb:113

I wouldn't rely on a comment for this. Maybe use a constant for both ruby and js code

Comments from Reviewable

* 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.

justin808 force-pushed the fix-performance-regression-parsing-string-props branch from 585d755 to 88d290e Compare April 24, 2017 23:34

justin808 changed the title ~~Fixes serious performance regression String props~~ Raise if versions differ and fix performance regression for string props Apr 25, 2017

justin808 force-pushed the fix-performance-regression-parsing-string-props branch 2 times, most recently from 7270158 to a0bbfd9 Compare April 25, 2017 10:21

justin808 force-pushed the fix-performance-regression-parsing-string-props branch from a0bbfd9 to 70ffd76 Compare April 25, 2017 10:25

justin808 requested a review from robwise April 25, 2017 10:33

justin808 added the review-needed label Apr 25, 2017

pjfitzgibbons reviewed Apr 25, 2017

View reviewed changes

kaizencodes reviewed Apr 25, 2017

View reviewed changes

justin808 force-pushed the fix-performance-regression-parsing-string-props branch from 70ffd76 to ef40698 Compare April 25, 2017 21:53

justin808 merged commit 251fa75 into master Apr 25, 2017

justin808 deleted the fix-performance-regression-parsing-string-props branch April 25, 2017 21:56

justin808 mentioned this pull request Apr 25, 2017

Performance decrease after 6.9 due to JSON.parse of already json string #819

Closed

artempetrovcode mentioned this pull request Jun 16, 2017

Fix incompatibility with react-on-rails v7.0.0+ Limenius/ReactBundle#23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise if versions differ and fix performance regression for string props #821

Raise if versions differ and fix performance regression for string props #821

justin808 commented Apr 24, 2017 •

edited

Loading

justin808 commented Apr 25, 2017

coveralls commented Apr 25, 2017 •

edited

Loading

coveralls commented Apr 25, 2017

coveralls commented Apr 25, 2017

pjfitzgibbons Apr 25, 2017

justin808 Apr 25, 2017

pjfitzgibbons Apr 25, 2017

kaizencodes Apr 25, 2017

kaizencodes Apr 25, 2017

kaizencodes Apr 25, 2017

pjfitzgibbons Apr 25, 2017

justin808 Apr 25, 2017

justin808 Apr 25, 2017

coveralls commented Apr 25, 2017 •

edited

Loading

mapreal19 commented Apr 26, 2017

Raise if versions differ and fix performance regression for string props #821

Raise if versions differ and fix performance regression for string props #821

Conversation

justin808 commented Apr 24, 2017 • edited Loading

Raise on Gem and Node Version Mismatch

Fix Performance Issue on String Props

justin808 commented Apr 25, 2017

coveralls commented Apr 25, 2017 • edited Loading

coveralls commented Apr 25, 2017

coveralls commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 25, 2017 • edited Loading

mapreal19 commented Apr 26, 2017

justin808 commented Apr 24, 2017 •

edited

Loading

coveralls commented Apr 25, 2017 •

edited

Loading

coveralls commented Apr 25, 2017 •

edited

Loading