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

[squadette]

See discussion in #660

Build passing: https://travis-ci.org/squadette/react_on_rails/builds/210095090

Thank you @eliaslopezgt for initial implementation.

---

This change is 

[coveralls]

Coverage remained the same at 97.898% when pulling f835c5f on squadette:script-tag into f42e8f4 on shakacode:master.

[justin808]

@squadette What's pending here?

[squadette]

Nothing, I guess it's ready for code review, fixes (if any) and merge.

[justin808]

@squadette Really awesome work.

My main concerns

1. That we changed a test that uses a JSON string to a hash. We need to support both. And be sure no XSS either way.
2. Why did a call to getAttributes change to attributes[key].value ?
3. Might need to deprecate the removal of a config value if this causes a crash.

---

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

---

app/helpers/react_on_rails_helper.rb, line 226 at r1 (raw file):

  def json_safe_and_pretty(something)
    if Rails.env.development?

Should this include Rails.env.test?

@robwise @alexfedoseev Any need to see the JSON look pretty in tests?

---

lib/react_on_rails/configuration.rb, line 128 at r1 (raw file):

      self.trace = trace.nil? ? Rails.env.development? : trace
      self.raise_on_prerender_error = raise_on_prerender_error
      self.skip_display_none = skip_display_none

Removing this may result in an error if this value is being set.

Please set this value in the react_on_rails.rb and see if there's a crash.

If so, let's leave this around with a deprecation message, and we'll remove with version 7.0.

---

node_package/src/clientStartup.js, line 50 at r1 (raw file):

function forEachComponent(fn, railsContext) {
  forEach(fn, 'js-react-on-rails-component', railsContext);

can we keep this a const at the top of the file?

---

node_package/src/clientStartup.js, line 55 at r1 (raw file):

function initializeStore(el, railsContext) {
  const context = findContext();
  const name = el.attributes['data-js-react-on-rails-store'].value;

how is el.attributes different than getAttribute?

---

node_package/src/clientStartup.js, line 63 at r1 (raw file):

function forEachStore(railsContext) {
  forEachByAttribute(initializeStore, 'data-js-react-on-rails-store', railsContext);

should we DRY up 'data-js-react-on-rails-store'?

---

spec/dummy/app/views/pages/server_side_hello_world_with_options.html.erb, line 30 at r1 (raw file):

<pre>
<%%= react_component("HelloWorld",
                    props: @app_props_server_render.to_json,

Sometimes we're passed props that are already a JSON string. Sometimes we're given props that are a Hash.

We need to be sure we handle both cases.

---

Comments from Reviewable

[eliaslopezgt]

👍 amazing work

[squadette]

Thank you. I've fixed two things, will fix the rest later.

---

Review status: 15 of 17 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

app/helpers/react_on_rails_helper.rb, line 226 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Should this include Rails.env.test?

@robwise @alexfedoseev Any need to see the JSON look pretty in tests?

Rewrote it to be pretty everywhere in non-production.

---

node_package/src/clientStartup.js, line 50 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

can we keep this a const at the top of the file?

I personally hate single-use constants because they inhibit grepping. I would prefer to leave it as, but of course I can change it.

---

node_package/src/clientStartup.js, line 55 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

how is el.attributes different than getAttribute?

getAttribute() is better, because it handles attributes which are not present. Fixed, thank you.

---

node_package/src/clientStartup.js, line 63 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

should we DRY up 'data-js-react-on-rails-store'?

I don't see a lot of value in micro-DRY. How would you suggest to change it?

---

Comments from Reviewable

[justin808]

Looking good! What's pending?

---

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

---

node_package/src/clientStartup.js, line 50 at r1 (raw file):

Previously, squadette (Alexey Mahotkin) wrote…

I personally hate single-use constants because they inhibit grepping. I would prefer to leave it as, but of course I can change it.

OK -- for single use. Keep as is. Name is self explanatory.

---

node_package/src/clientStartup.js, line 63 at r1 (raw file):

Previously, squadette (Alexey Mahotkin) wrote…

I don't see a lot of value in micro-DRY. How would you suggest to change it?

If constant value is in two places, then declare at top of file as const

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 97.898% when pulling 807e140 on squadette:script-tag into f42e8f4 on shakacode:master.

[justin808]

@squadette Do you need any help from me on this one? I want to get this one shipped, and to then get started on v7 with Webpacker.

[justin808]

@squadette We've got to get this one wrapped up. Any volunteers to get this one across the finish line?

[justin808]

Work is now going into #775. @squadette Thanks for your help so far, and please help us review #775.