Improve json conversion with tests and support for older Rails 3.x #787

cheremukhin23 · 2017-03-31T12:07:03Z

Add error handling when unable to parse json
Add tests to ensure json_safe_and_pretty handles hashes and strings

This change is

coveralls · 2017-03-31T12:20:39Z

Coverage increased (+0.003%) to 97.925% when pulling 5867246 on cheremukhin23:improve-json-safe-and-pretty into 5954e36 on shakacode:master.

coveralls · 2017-04-01T08:27:28Z

Coverage increased (+0.01%) to 97.935% when pulling a3ef532 on cheremukhin23:improve-json-safe-and-pretty into 5954e36 on shakacode:master.

justin808 · 2017-04-01T08:57:12Z

Looking great!

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

app/helpers/react_on_rails_helper.rb, line 223 at r2 (raw file):

  end

  def json_safe_and_pretty(hash_or_string)

Can we throw here if the value passed is not a hash or a string? What else could the value be?

app/helpers/react_on_rails_helper.rb, line 231 at r2 (raw file):

      escape_json(hash_value.to_json)
    end
  rescue JSON::ParserError => e

Can we move this catch block around the JSON.parse above?

we can do something like:

  hash_value = if hash_or_string.is_a?(String) 
                          begin
                              JSON.parse(hash_or_string)
                           rescue JSON::ParserError => e
                               raise e, %(Cannot parse #{hash_or_string} \n Backtrace: #{e.backtrace.join("\n")})
                          else
                              hash_or_string
                          end

app/helpers/react_on_rails_helper.rb, line 242 at r2 (raw file):

  end

  def old_json_escape(json)

Can you provide a comment with a link on to the github source of this?

spec/dummy/spec/helpers/react_on_rails_helper_spec.rb, line 40 at r2 (raw file):

      expect(escaped_json).to eq(hash_sanitized)
    end
  end

Can we add a couple tests that confirm the exceptions:

non-hash, non-string passed
non parseable string in development mode

Also, we're never testing the part where we check we're in development mode.

I'd try to mock/stub out the Rails.env for that test.

# RSpec version
it 'tests my code' do
  allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new('development'))
  # test code
end

Some options from Google

Comments from Reviewable

justin808 · 2017-04-02T01:21:41Z

See note below. I might just release a temp patch on this.

Review status: all files reviewed at latest revision, 5 unresolved discussions.

app/helpers/react_on_rails_helper.rb, line 224 at r2 (raw file):

  def json_safe_and_pretty(hash_or_string)
    hash_value = hash_or_string.is_a?(String) ? JSON.parse(hash_or_string) : hash_or_string

djudji [1:57 AM]
OK, so this is the thing, the code brakes at AppointmentsList component, after the update. It gives an appointments.map is not a function error, which means that it can not iterate over the Object type (edited)

[1:59]
@justin any recommendations?

djudji [3:31 AM]
could it be a new way of passing data from Rails to React (as I am testing more and more, it looks like props is not getting the right values from a controller)?

djudji [3:53 AM]
it hangs inside function render(el, railsContext) in clientStartup.js

[3:55]
this is how I am sending the data to a React component
react_component 'Appointments', props: {appointments: @appointments}

[3:56]
and on "the other side" I get
Object {appointments: "#<Appointment::ActiveRecord_Relation:0x007f972440cb90>"}
instead of an array of Objects, like before updating to 6.9.1 (edited)

justin [8:16 AM]
Dev or production. Please try both.

djudji [9:49 AM]
Currently on dev. Will try it in production again (first app didn't work because I haven't set a new version for react_on_rails in package.json)

OK, this is a bit strange.

I cloned non-updated app
I did upgrade to react_on_rails 6.9.1
Created a new heroku app and pushed the upgraded app to Heroku.

Now everything is working fine in production, but in dev I still have the same error.

This is the URL -> https://calendar-react-on-rails.herokuapp.com/

I can confirm that both of the apps work when pushed to production.

This is the first URL -> https://calendar-railsreact.herokuapp.com/ (edited

the error is in the conversion to the pretty JSON… for development mode.

   if Rails.env.development?
     # TODO: for json_safe_and_pretty
     # 1. Add test
     # 2. Add error handler if cannot parse the string with nice message
     # 3. Consider checking that if not a string then a Hash
     hash_value = hash_or_string.is_a?(String) ? JSON.parse(hash_or_string) : hash_or_string
     ERB::Util.json_escape(JSON.pretty_generate(hash_value))
   else
     ERB::Util.json_escape(hash_or_string.to_json)
   end
 end

Before, we used to call:

    props.is_a?(String) ? json_escape(props) : props.to_json
  end```


So I think if a Hash is passed, rather than a string, and we want to pretty print, then we’d have to go:

1. Hash => to_json (converts active record objects) => String
2. String => Hash
3. Then call `ERB::Util.json_escape(JSON.pretty_generate(hash_value))`

This could be a little slow.

1. Wondering if this should be optional, in `config/initializers/react_on_rails.rb`
2. Another option is skip pretty printing completely (but then should make an option).

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/shakacode/react_on_rails/787#-:-KggFA9_7z0FTL11MGXB:b-u1x6lu)*
<!-- Sent from Reviewable.io -->

justin808 · 2017-04-02T07:35:12Z

Note #789 is removing the pretty formatting.

We can add it back in...

justin808 · 2017-04-02T07:56:16Z

@cheremukhin23 I thought more about the pretty formatting of json.

Let's add a configuration flag for this, since it tends to break...

Default is false.

# Pretty printing props looks nice when you view the page source. However, if you have a large
# amount of props data, the page rendering can be a little slower.
config.pretty_print_props_in_development = false

justin808 · 2017-04-03T06:35:37Z

See #791. I fixed another critical bug...

[1] (pry) #<#<Class:0x007f84e5bd19f8>>: 0> puts hash_or_string.to_json
"{\"comments\":[{\"id\":9,\"author\":\"xxx\",\"text\":\"xxxxxx\",\"created_at\":\"2017-01-11T08:14:10.296Z\",\"updated_at\":\"2017-01-11T08:14:10.296Z\"},

[2] (pry) #<Class:0x007f84e5bd19f8>>: 0> puts hash_or_string
{"comments":[{"id":9,"author":"xxx","text":"xxxxxx","created_at":"2017-01-11T08:14:10.296Z","updated_at":"2017-01-11T08:14:10.296Z"},

justin808 · 2017-04-10T04:58:39Z

See #799

…into improve-json-safe-and-pretty # Conflicts: # app/helpers/react_on_rails_helper.rb

cheremukhin23 · 2017-04-11T14:20:48Z

Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions.

app/helpers/react_on_rails_helper.rb, line 223 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Can we throw here if the value passed is not a hash or a string? What else could the value be?

We discard this code

app/helpers/react_on_rails_helper.rb, line 224 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

djudji [1:57 AM]
OK, so this is the thing, the code brakes at AppointmentsList component, after the update. It gives an appointments.map is not a function error, which means that it can not iterate over the Object type (edited)

[1:59]
@justin any recommendations?

djudji [3:31 AM]
could it be a new way of passing data from Rails to React (as I am testing more and more, it looks like props is not getting the right values from a controller)?

djudji [3:53 AM]
it hangs inside function render(el, railsContext) in clientStartup.js

[3:55]
this is how I am sending the data to a React component
react_component 'Appointments', props: {appointments: @appointments}

[3:56]
and on "the other side" I get
Object {appointments: "#<Appointment::ActiveRecord_Relation:0x007f972440cb90>"}
instead of an array of Objects, like before updating to 6.9.1 (edited)

justin [8:16 AM]
Dev or production. Please try both.

djudji [9:49 AM]
Currently on dev. Will try it in production again (first app didn't work because I haven't set a new version for react_on_rails in package.json)

OK, this is a bit strange.

I cloned non-updated app

I did upgrade to react_on_rails 6.9.1

Created a new heroku app and pushed the upgraded app to Heroku.

Now everything is working fine in production, but in dev I still have the same error.

This is the URL -> https://calendar-react-on-rails.herokuapp.com/

I can confirm that both of the apps work when pushed to production.

This is the first URL -> https://calendar-railsreact.herokuapp.com/ (edited

the error is in the conversion to the pretty JSON… for development mode.
   if Rails.env.development?
     # TODO: for json_safe_and_pretty
     # 1. Add test
     # 2. Add error handler if cannot parse the string with nice message
     # 3. Consider checking that if not a string then a Hash
     hash_value = hash_or_string.is_a?(String) ? JSON.parse(hash_or_string) : hash_or_string
     ERB::Util.json_escape(JSON.pretty_generate(hash_value))
   else
     ERB::Util.json_escape(hash_or_string.to_json)
   end
 end
Before, we used to call:
    props.is_a?(String) ? json_escape(props) : props.to_json
  end```


So I think if a Hash is passed, rather than a string, and we want to pretty print, then we’d have to go:

1. Hash => to_json (converts active record objects) => String
2. String => Hash
3. Then call `ERB::Util.json_escape(JSON.pretty_generate(hash_value))`

This could be a little slow.

1. Wondering if this should be optional, in `config/initializers/react_on_rails.rb`
2. Another option is skip pretty printing completely (but then should make an option).
</blockquote></details>

currently decided not to use pretty generate

---

*[app/helpers/react_on_rails_helper.rb, line 231 at r2](https://reviewable.io:443/reviews/shakacode/react_on_rails/787#-KgcggrDdvDqTLt3uR8q:-KhSNfG5p3IgHg_wp0bx:b-vjole3) ([raw file](https://github.com/shakacode/react_on_rails/blob/a3ef532d26039dc87d3032142a3171a6d26b2f3a/app/helpers/react_on_rails_helper.rb#L231)):*
<details><summary><i>Previously, justin808 (Justin Gordon) wrote…</i></summary><blockquote>

Can we move this catch block around the JSON.parse above?

we can do something like:

```ruby
  hash_value = if hash_or_string.is_a?(String) 
                          begin
                              JSON.parse(hash_or_string)
                           rescue JSON::ParserError => e
                               raise e, %(Cannot parse #{hash_or_string} \n Backtrace: #{e.backtrace.join("\n")})
                          else
                              hash_or_string
                          end  

discarded code

app/helpers/react_on_rails_helper.rb, line 242 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Can you provide a comment with a link on to the github source of this?

added link

spec/dummy/spec/helpers/react_on_rails_helper_spec.rb, line 40 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Can we add a couple tests that confirm the exceptions:

non-hash, non-string passed

non parseable string in development mode

Also, we're never testing the part where we check we're in development mode.

I'd try to mock/stub out the Rails.env for that test.
# RSpec version
it 'tests my code' do
  allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new('development'))
  # test code
end
Some options from Google

added test for arguments

Comments from Reviewable

justin808 · 2017-04-11T18:49:58Z

We need to fix this deprecation for React.

====> React On Rails: Checking app/assets/webpack for outdated/missing bundles
You're running an old version of PhantomJS, update to >= 2.1.1 for a better experience.
  the hello world example works (FAILED - 1)
Failures:
  1) Hello World the hello world example works
     Failure/Error: visit "/hello_world"
     
     Capybara::Poltergeist::JavascriptError:
       One or more errors were raised in the Javascript code on the page. If you don't care about these errors, you can ignore them by setting js_errors: false in your Poltergeist configuration (see documentation for details).
     
       Warning: Accessing PropTypes via the main React package is deprecated. Use the prop-types package from npm instead.
       Warning: Accessing PropTypes via the main React package is deprecated. Use the prop-types package from npm instead.
           at :36 in printWarning
     # /home/travis/.rvm/gems/ruby-2.2.6/gems/poltergeist-1.13.0/lib/capybara/poltergeist/browser.rb:376:in `command'
     # /home/travis/.rvm/gems/ruby-2.2.6/gems/poltergeist-1.13.0/lib/capybara/poltergeist/browser.rb:35:in `visit'
     # /home/travis/.rvm/gems/ruby-2.2.6/gems/poltergeist-1.13.0/lib/capybara/poltergeist/driver.rb:97:in `visit'
     # /home/travis/.rvm/gems/ruby-2.2.6/gems/capybara-2.12.1/lib/capybara/session.rb:252:in `visit'
     # /home/travis/.rvm/gems/ruby-2.2.6/gems/capybara-2.12.1/lib/capybara/dsl.rb:52:in `block (2 levels) in <module:DSL>'
     # ./spec/features/hello_world_spec.rb:5:in `block (2 levels) in <top (required)>'
     # /home/travis/.rvm/gems/ruby-2.2.6/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:112:in `block in run'
     # /home/travis/.rvm/gems/ruby-2.2.6/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:101:in `loop'
     # /home/travis/.rvm/gems/ruby-2.2.6/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:101:in `run'
     # /home/travis/.rvm/gems/ruby-2.2.6/gems/rspec-retry-0.5.3/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # /home/travis/.rvm/gems/ruby-2.2.6/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:30:in `block (2 levels) in setup'
Finished in 2.4 seconds (files took 2.58 seconds to load)
1 example, 1 failure

justin808 · 2017-04-11T18:56:23Z

Looking good:

Few comments.
Need to pass CI. That could be a separate PR on master
Need a CHANGELOG.md entry.

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

app/helpers/react_on_rails_helper.rb, line 234 at r3 (raw file):

  def escape_json(json)
    return old_json_escape(json) unless Rails::VERSION::STRING >= "4"

I think this not right. What if the Rails version is "10.0".

Please google for a proper version comparator.

Something like this (you creating the rails_version_less_than

 return old_json_escape(json) if rails_version_less_than(4.0)

Comments from Reviewable

justin808 · 2017-04-13T06:40:32Z

@cheremukhin23 See comment: #799 (comment)

justin808 · 2017-04-13T06:41:17Z

@cheremukhin23 @squadette Maybe I should merge this one and let's add the feature in a new PR?

justin808 · 2017-04-13T08:16:28Z

please rebase on master and that should fix the CI, @cheremukhin23

…into improve-json-safe-and-pretty # Conflicts: # app/helpers/react_on_rails_helper.rb

coveralls · 2017-04-13T15:48:05Z

Coverage increased (+0.02%) to 97.997% when pulling 78a1c3c on cheremukhin23:improve-json-safe-and-pretty into d5d579e on shakacode:master.

coveralls · 2017-04-13T15:48:05Z

Coverage increased (+0.02%) to 97.997% when pulling 78a1c3c on cheremukhin23:improve-json-safe-and-pretty into d5d579e on shakacode:master.

cheremukhin23 · 2017-04-13T16:05:30Z

Review status: 1 of 3 files reviewed at latest revision, 1 unresolved discussion.

app/helpers/react_on_rails_helper.rb, line 234 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I think this not right. What if the Rails version is "10.0".

Please google for a proper version comparator.

Something like this (you creating the rails_version_less_than
 return old_json_escape(json) if rails_version_less_than(4.0)

Used Gem::Version to compare versions

Comments from Reviewable

coveralls · 2017-04-13T16:06:37Z

Coverage increased (+0.02%) to 97.997% when pulling 19722db on cheremukhin23:improve-json-safe-and-pretty into d5d579e on shakacode:master.

justin808 · 2017-04-13T18:47:05Z

Very close. But missing tests on part of the PR.

Reviewed 1 of 1 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

app/helpers/react_on_rails_helper.rb, line 251 at r6 (raw file):

    json_escape = { "&" => '\u0026', ">" => '\u003e', "<" => '\u003c', "\u2028" => '\u2028', "\u2029" => '\u2029' }
    json_escape_regexp = /[\u2028\u2029&><]/u
    json.to_s.gsub(json_escape_regexp, json_escape)

This code is not tested. We need a test to confirm this method works as well as the method rails_version_less_than.

Please add a couple simple tests for rails_version_less_than with cases "3.2", "3", "4.2", "10.0".

Comments from Reviewable

justin808 · 2017-04-16T09:37:30Z

@cheremukhin23 Please let me know if you can finish this. If you don't have time, I need to find an alternate resource.

@squadette, any chance that you can help with this one?

justin808 · 2017-04-18T02:36:27Z

@Ynote, is there chance that you can give us a hand with this one?

djfdev · 2017-04-18T08:00:35Z

app/helpers/react_on_rails_helper.rb

@@ -224,8 +224,33 @@ def server_render_js(js_expression, options = {})
    # rubocop:enable Style/RaiseArgs
  end

+  def json_safe_and_pretty(hash_or_string)
+    unless hash_or_string.class.in?([Hash, String])
+      raise "#{hash_or_string.class} is unsupported argument class for this method"


I would name the types that are accepted by the method in this error.

Ynote · 2017-04-18T10:17:02Z

Hi,

I can help on the PR. @cheremukhin23, can you give me permission on the PR so I can fetch the branch and add the missing tests?

justin808 · 2017-04-18T21:46:25Z

@Ynote @cheremukhin23 You can also pull his changes into your own fork and make a new branch.

@squadette would like us to put back in JSON props as pretty. I'm ok with that either in this or a different PR, but I won't accept any PR without full test coverage and documentation changes.

justin808 · 2017-04-21T07:39:32Z

See #812.

…ails older than 4.1 (#812) * Improve json_safe_and_pretty method to handle parse error * Add tests to json_safe_and_pretty method * Add old json escape method for Rails versions less than 4 * Add json safe and pretty argument class test * Add rails version less than method * Add tests for json encoding helper methods * Move rails_version_less_than method into Utils module * Extract json escaping logic into JsonOutput class * Remove "or equal" logic from rails_version_less_than util method * Use monkey patch json escape from Rails 4.2 instead of Rails 4 * Memoize result for static method rails_version_less_than * Update json escaping method into class methods

cheremukhin23 added 2 commits March 31, 2017 15:03

Improve json_safe_and_pretty method to handle parse error

703e1d7

Add tests to json_safe_and_pretty method

5867246

cheremukhin23 added 2 commits April 1, 2017 10:57

Add old json escape method for Rails versions less than 4

68044df

Fix rubocop offenses

a3ef532

justin808 mentioned this pull request Apr 2, 2017

Don't pretty format JSON in development #789

Merged

This was referenced Apr 2, 2017

[Rails 3.2][v6.9.0] Parsing error when fetching ReactOnRails context from script tag #782

Closed

Don't call to_json on string props #791

Merged

justin808 mentioned this pull request Apr 10, 2017

Putting back in pretty printing of JSON props? #799

Closed

cheremukhin23 added 3 commits April 11, 2017 17:04

Merge branch 'master' of https://github.com/shakacode/react_on_rails …

fb697cd

…into improve-json-safe-and-pretty # Conflicts: # app/helpers/react_on_rails_helper.rb

Fixed rubocop offense

80d3dbc

Add json safe and pretty argument class test

358d572

Add rails version less than method

5ad43e8

justin808 changed the title ~~Improve json safe and pretty~~ Improve json conversion with tests and support for older Rails 3.x Apr 13, 2017

Merge branch 'master' of https://github.com/shakacode/react_on_rails …

78a1c3c

…into improve-json-safe-and-pretty # Conflicts: # app/helpers/react_on_rails_helper.rb

Update CHANGELOG.md

19722db

djfdev reviewed Apr 18, 2017

View reviewed changes

Ynote mentioned this pull request Apr 19, 2017

[Extends #787] - Improve json conversion with tests and support for Rails older than 4.1 #812

Merged

justin808 closed this Apr 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve json conversion with tests and support for older Rails 3.x #787

Improve json conversion with tests and support for older Rails 3.x #787

cheremukhin23 commented Mar 31, 2017 •

edited by justin808

Loading

coveralls commented Mar 31, 2017 •

edited

Loading

coveralls commented Apr 1, 2017 •

edited

Loading

justin808 commented Apr 1, 2017

justin808 commented Apr 2, 2017

justin808 commented Apr 2, 2017

justin808 commented Apr 2, 2017

justin808 commented Apr 3, 2017

justin808 commented Apr 10, 2017

cheremukhin23 commented Apr 11, 2017

justin808 commented Apr 11, 2017

justin808 commented Apr 11, 2017

justin808 commented Apr 13, 2017

justin808 commented Apr 13, 2017

justin808 commented Apr 13, 2017

coveralls commented Apr 13, 2017

coveralls commented Apr 13, 2017

cheremukhin23 commented Apr 13, 2017

coveralls commented Apr 13, 2017 •

edited

Loading

justin808 commented Apr 13, 2017

justin808 commented Apr 16, 2017

justin808 commented Apr 18, 2017

djfdev Apr 18, 2017

Ynote commented Apr 18, 2017

justin808 commented Apr 18, 2017

justin808 commented Apr 21, 2017

Improve json conversion with tests and support for older Rails 3.x #787

Improve json conversion with tests and support for older Rails 3.x #787

Conversation

cheremukhin23 commented Mar 31, 2017 • edited by justin808 Loading

coveralls commented Mar 31, 2017 • edited Loading

coveralls commented Apr 1, 2017 • edited Loading

justin808 commented Apr 1, 2017

justin808 commented Apr 2, 2017

justin808 commented Apr 2, 2017

justin808 commented Apr 2, 2017

justin808 commented Apr 3, 2017

justin808 commented Apr 10, 2017

cheremukhin23 commented Apr 11, 2017

justin808 commented Apr 11, 2017

justin808 commented Apr 11, 2017

justin808 commented Apr 13, 2017

justin808 commented Apr 13, 2017

justin808 commented Apr 13, 2017

coveralls commented Apr 13, 2017

coveralls commented Apr 13, 2017

cheremukhin23 commented Apr 13, 2017

coveralls commented Apr 13, 2017 • edited Loading

justin808 commented Apr 13, 2017

justin808 commented Apr 16, 2017

justin808 commented Apr 18, 2017

djfdev Apr 18, 2017

Choose a reason for hiding this comment

Ynote commented Apr 18, 2017

justin808 commented Apr 18, 2017

justin808 commented Apr 21, 2017

cheremukhin23 commented Mar 31, 2017 •

edited by justin808

Loading

coveralls commented Mar 31, 2017 •

edited

Loading

coveralls commented Apr 1, 2017 •

edited

Loading

coveralls commented Apr 13, 2017 •

edited

Loading