Use <script type="application/json"> for props and store #775

cheremukhin23 · 2017-03-27T20:24:48Z

See discussion in #760

Change code according to unresolved discussions in #760

This change is

…ribute Initial implementation by Elias Lopez Gutierrez <[email protected]>

… present.

This reverts commit 8492f0e.

…ing the both cases

…into script-tag

justin808 · 2017-03-27T21:39:48Z

Looks excellent! Just a couple small comments.

Reviewed 13 of 17 files at r1, 2 of 4 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

lib/react_on_rails/react_component/options.rb, line 9 at r2 (raw file):

      NO_PROPS = {}.freeze
      HIDDEN = "display:none".freeze

I think we can remove HIDDEN

lib/react_on_rails/react_component/options.rb, line 57 at r2 (raw file):

      end

      def style

We can probably remove style since we're removing HIDDEN

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

    let(:id) { "App-react-component-0" }

    # rubocop:disable Metrics/LineLength

You can break the string with \ at the end, so the line is not crazy long, and remove the lint disable

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

      let(:react_definition_script) do
        # rubocop:disable Metrics/LineLength

try to avoid if possible

Comments from Reviewable

justin808 · 2017-03-27T21:41:41Z

@cheremukhin23: CI failures:

Failures:
  1) Pages/Index Server Rendering with Options changes name in message according to input
     Failure/Error: is_expected.to have_content new_text
       expected to find text "John Doe" in "Hello, Mr. Server Side Rendering!"
     Shared Example Group: "React Component" called from ./spec/features/integration_spec.rb:61
     # ./spec/features/integration_spec.rb:9:in `block (2 levels) in change_text_expect_dom_selector'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/capybara-2.12.1/lib/capybara/session.rb:310:in `within'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/capybara-2.12.1/lib/capybara/dsl.rb:52:in `block (2 levels) in <module:DSL>'
     # ./spec/features/integration_spec.rb:8:in `block in change_text_expect_dom_selector'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/capybara-2.12.1/lib/capybara/session.rb:310:in `within'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/capybara-2.12.1/lib/capybara/dsl.rb:52:in `block (2 levels) in <module:DSL>'
     # ./spec/features/integration_spec.rb:6:in `change_text_expect_dom_selector'
     # ./spec/features/integration_spec.rb:18:in `block (2 levels) in <top (required)>'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:112:in `block in run'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:101:in `loop'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:101:in `run'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/rspec-retry-0.5.3/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:30:in `block (2 levels) in setup'
  2) Turbolinks across pages changes name in message according to input
     Failure/Error: is_expected.to have_content new_text
       expected to find text "John Doe" in "Hello, Mr. Server Side Rendering!"
     # ./spec/features/integration_spec.rb:9:in `block (2 levels) in change_text_expect_dom_selector'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/capybara-2.12.1/lib/capybara/session.rb:310:in `within'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/capybara-2.12.1/lib/capybara/dsl.rb:52:in `block (2 levels) in <module:DSL>'
     # ./spec/features/integration_spec.rb:8:in `block in change_text_expect_dom_selector'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/capybara-2.12.1/lib/capybara/session.rb:310:in `within'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/capybara-2.12.1/lib/capybara/dsl.rb:52:in `block (2 levels) in <module:DSL>'
     # ./spec/features/integration_spec.rb:6:in `change_text_expect_dom_selector'
     # ./spec/features/integration_spec.rb:72:in `block (2 levels) in <top (required)>'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:112:in `block in run'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:101:in `loop'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:101:in `run'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/rspec-retry-0.5.3/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/rspec-retry-0.5.3/lib/rspec/retry.rb:30:in `block (2 levels) in setup'
Finished in 49.89 seconds (files took 5.38 seconds to load)
72 examples, 2 failures
Failed examples:
rspec ./spec/features/integration_spec.rb[1:2:2] # Pages/Index Server Rendering with Options changes name in message according to input
rspec ./spec/features/integration_spec.rb:68 # Turbolinks across pages changes name in message according to input

cheremukhin23 · 2017-03-28T10:50:27Z

Review status: 14 of 15 files reviewed at latest revision, 4 unresolved discussions.

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

Previously, justin808 (Justin Gordon) wrote…

You can break the string with \ at the end, so the line is not crazy long, and remove the lint disable

Deleted rubocop exclusions and formatted string. Since we use squish on string, we can just format the string to be readable

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

Previously, justin808 (Justin Gordon) wrote…

try to avoid if possible

Deleted rubocop exclusion

Comments from Reviewable

…ent module

cheremukhin23 · 2017-03-28T12:03:57Z

Review status: 13 of 15 files reviewed at latest revision, 4 unresolved discussions.

lib/react_on_rails/react_component/options.rb, line 9 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I think we can remove HIDDEN

Removed it

lib/react_on_rails/react_component/options.rb, line 57 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

We can probably remove style since we're removing HIDDEN

Removed it

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

Previously, cheremukhin23 (Dmitriy Cheremukhin) wrote…

Deleted rubocop exclusions and formatted string. Since we use squish on string, we can just format the string to be readable

Changed format to single quotes and \

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

Previously, cheremukhin23 (Dmitriy Cheremukhin) wrote…

Deleted rubocop exclusion

Changed format to single quotes and \

Comments from Reviewable

cheremukhin23 · 2017-03-28T14:50:30Z

Currently CI fails because the lib can't handle props: @app_props_server_render.to_json , only props: @app_props_server_render. It seems it happens because of code in clientStartup.js.
props = JSON.parse(el.textContent) can't be properly parsed
Working on this issue now

… to a string

coveralls · 2017-03-28T21:23:22Z

Coverage increased (+0.006%) to 97.903% when pulling 09b03e5 on cheremukhin23:script-tag into cca1351 on shakacode:master.

cheremukhin23 · 2017-03-28T21:31:13Z

@justin808 All checks pass now

justin808 · 2017-03-29T08:09:25Z

One really important comment:

I think that the conversion of the string JSON props to an object should happen on the Rails side, so that there's no need to double parse on the client side.

We also need a unit test to confirm that a XSS cannot be introduced if the JSON string is passed, rather than object.

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

node_package/src/clientStartup.js, line 101 at r4 (raw file):

  const context = findContext();
  const jsonEl = JSON.parse(el.textContent, (key, value) =>
    (key === 'props' && typeof value === 'string' ? JSON.parse(value) : value),

I think that the conversion of the string JSON props to an object should happen on the Rails side, so that there's no need to double parse on the client side.

We also need a unit test to confirm that a XSS cannot be introduced if the JSON string is passed, rather than object.

Comments from Reviewable

cheremukhin23 · 2017-03-29T20:14:34Z

Moved this part to the Rails side

Review status: 11 of 15 files reviewed at latest revision, 3 unresolved discussions.

node_package/src/clientStartup.js, line 101 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I think that the conversion of the string JSON props to an object should happen on the Rails side, so that there's no need to double parse on the client side.

We also need a unit test to confirm that a XSS cannot be introduced if the JSON string is passed, rather than object.

Moved it to the rails side to ReactOnRails::ReactComponent::Options. Now we parse all incoming props to hash. Wrote test to ensure we have sanitized react component after passing props as json string

Comments from Reviewable

cheremukhin23 · 2017-03-29T20:25:18Z

@justin808 Should work after build is passed. Any comments on last improvements?

coveralls · 2017-03-29T20:37:33Z

Coverage increased (+0.008%) to 97.906% when pulling d186aa1 on cheremukhin23:script-tag into cca1351 on shakacode:master.

coveralls · 2017-03-29T20:47:25Z

Coverage increased (+0.008%) to 97.906% when pulling d186aa1 on cheremukhin23:script-tag into cca1351 on shakacode:master.

coveralls · 2017-03-29T20:47:26Z

Coverage increased (+0.008%) to 97.906% when pulling d186aa1 on cheremukhin23:script-tag into cca1351 on shakacode:master.

justin808 · 2017-03-29T21:10:59Z

Looks EXCELLENT!

We just need the CHANGELOG.md entry and I'll release this.

Reviewed 4 of 4 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

justin808 · 2017-03-30T08:53:59Z

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

coveralls · 2017-03-30T09:01:48Z

Coverage increased (+0.008%) to 97.906% when pulling 82adca1 on cheremukhin23:script-tag into cca1351 on shakacode:master.

squadette · 2017-04-02T17:16:49Z

Hi, sorry for disappearance.

I've upgraded my project to react-on-rails to 6.9.2 and tested, it works perfectly. Thanks for finishing that!

justin808 · 2017-04-06T08:11:01Z

@squadette Please update to 6.9.3!

squadette and others added 10 commits March 11, 2017 18:54

Fix webpack.config.js for Procfile.hot and Webpack 2

0c3409e

Refactor console output to use <script> tag with a certain ID

bc3f197

Use <script type="application/json"> with JSON content instead of att…

f835c5f

…ribute Initial implementation by Elias Lopez Gutierrez <[email protected]>

Pretty-print JSON in development and testing

8492f0e

getAttribute() is better, because it handles attributes which are not…

12f88a5

… present.

Revert "Pretty-print JSON in development and testing"

807e140

This reverts commit 8492f0e.

Return skip display none to react on rails configuration

cdd8722

Extract data-js-react-on-rails-store attribute to the const

a5a414a

Return to_json method call to app_props_server_render to ensure handl…

27bfe25

…ing the both cases

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

2486892

…into script-tag

cheremukhin23 force-pushed the script-tag branch from 38b5f77 to 2486892 Compare March 27, 2017 20:28

cheremukhin23 changed the title ~~Use <script type="application/json"> for props and store~~ WIP: Use <script type="application/json"> for props and store Mar 27, 2017

Change const name

c64db94

justin808 mentioned this pull request Mar 28, 2017

Use <script type="application/json"> for props and store #760

Closed

Remove rubocop exclusions to line length

d4947b7

cheremukhin23 added 2 commits March 28, 2017 14:03

Delete unnecessary style method and HIDDEN constant from react compon…

9a76dc2

…ent module

Format strings to pass the tests

0975f04

cheremukhin23 added 3 commits March 28, 2017 23:19

Improve jsonEl parsing to handle props as already converted from JSON…

cb3c9d0

… to a string

Update code to pass linter

949f501

Fix rubocop offenses

09b03e5

cheremukhin23 added 2 commits March 29, 2017 21:28

Move handling props already converted to json string to ruby code

25bb18c

Escape json from props as json string

857f7d8

cheremukhin23 added 2 commits March 29, 2017 23:02

Add test to ensure that even json string props are sanitezed

202f3c0

Delete test json props because we parse all props to a hash now

d186aa1

cheremukhin23 changed the title ~~WIP: Use <script type="application/json"> for props and store~~ Use <script type="application/json"> for props and store Mar 30, 2017

Update CHANGELOG.md

82adca1

justin808 merged commit 7ccd603 into shakacode:master Mar 30, 2017

nacmartin mentioned this pull request Mar 30, 2017

Update to react-on-rails 6.6 when ready Limenius/symfony-react-sandbox#25

Closed

justin808 mentioned this pull request Apr 2, 2017

Change div tags to script tags and put viewContext/props inside #412

Closed

teameh mentioned this pull request Apr 7, 2017

Fix incompatibility with react-on-rails npm package Limenius/ReactBundle#20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use <script type="application/json"> for props and store #775

Use <script type="application/json"> for props and store #775

cheremukhin23 commented Mar 27, 2017 •

edited by justin808

Loading

justin808 commented Mar 27, 2017

justin808 commented Mar 27, 2017

cheremukhin23 commented Mar 28, 2017

cheremukhin23 commented Mar 28, 2017

cheremukhin23 commented Mar 28, 2017

coveralls commented Mar 28, 2017 •

edited

Loading

cheremukhin23 commented Mar 28, 2017

justin808 commented Mar 29, 2017

cheremukhin23 commented Mar 29, 2017

cheremukhin23 commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

justin808 commented Mar 29, 2017

justin808 commented Mar 30, 2017

coveralls commented Mar 30, 2017

squadette commented Apr 2, 2017

justin808 commented Apr 6, 2017

Use <script type="application/json"> for props and store #775

Use <script type="application/json"> for props and store #775

Conversation

cheremukhin23 commented Mar 27, 2017 • edited by justin808 Loading

justin808 commented Mar 27, 2017

justin808 commented Mar 27, 2017

cheremukhin23 commented Mar 28, 2017

cheremukhin23 commented Mar 28, 2017

cheremukhin23 commented Mar 28, 2017

coveralls commented Mar 28, 2017 • edited Loading

cheremukhin23 commented Mar 28, 2017

justin808 commented Mar 29, 2017

cheremukhin23 commented Mar 29, 2017

cheremukhin23 commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

justin808 commented Mar 29, 2017

justin808 commented Mar 30, 2017

coveralls commented Mar 30, 2017

squadette commented Apr 2, 2017

justin808 commented Apr 6, 2017

cheremukhin23 commented Mar 27, 2017 •

edited by justin808

Loading

coveralls commented Mar 28, 2017 •

edited

Loading