Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change div tags to script tags for props #411

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,4 @@ gem "chromedriver-helper"
gem "launchy"
gem "poltergeist"
gem "selenium-webdriver"
gem "yajl-ruby"
48 changes: 26 additions & 22 deletions app/helpers/react_on_rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
# 2. Keep all #{some_var} fully to the left so that all indentation is done evenly in that var
require "react_on_rails/prerender_error"
require "addressable/uri"
require "yajl"

module ReactOnRailsHelper
# The env_javascript_include_tag and env_stylesheet_link_tag support the usage of a webpack
Expand Down Expand Up @@ -88,6 +89,7 @@ def env_stylesheet_link_tag(args = {})
# raise_on_prerender_error: <true/false> Default to false. True will raise exception on server
# if the JS code throws
# Any other options are passed to the content tag, including the id.
# rubocop:disable Metrics/AbcSize
def react_component(component_name, raw_options = {})
# Create the JavaScript and HTML to allow either client or server rendering of the
# react_component.
Expand All @@ -102,11 +104,13 @@ def react_component(component_name, raw_options = {})
# The reason is that React is smart about not doing extra work if the server rendering did its job.

component_specification_tag =
content_tag(:div,
"",
content_tag(:script,
class: "js-react-on-rails-component",
style: options.style,
data: options.data)
style: nil,
data: options.data) do
props = Yajl.dump(options.props.is_a?(String) ? JSON.parse(options.props) : options.props)
"var #{options.dom_id.tr('-', '_')} = #{props};".html_safe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest not playing games with global Javascript variables but instead use

<script type="application/json" id="...">{ "json": "encoded-data" }</script>

Also, it would really help development if you use human-readable JSON in non-development environment, a-la:

  def json_safe_and_pretty(something)
    if Rails.env.development?
      json_escape(JSON.pretty_generate(something.as_json))
    else
      json_escape(something.to_json)
    end
  end

Copy link
Contributor

@squadette squadette Nov 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also looks like JSON <script> tag also solves the Turbolinks issue, because there is no need to execute any <script> tag, you just have to replace el.getAttribute() with (IIRC) .textContent. Also, no eval()!

end

# Create the HTML rendering part
result = server_rendered_react_component_html(options.props, options.name, options.dom_id,
Expand All @@ -126,12 +130,12 @@ def react_component(component_name, raw_options = {})

# IMPORTANT: Ensure that we mark string as html_safe to avoid escaping.
result = <<-HTML.html_safe
#{component_specification_tag}
#{component_specification_tag}
#{rendered_output}
#{options.replay_console ? console_script : ''}
HTML

prepend_render_rails_context(result)
add_render_rails_context(result)
end

# Separate initialization of store from react_component allows multiple react_component calls to
Expand All @@ -154,7 +158,7 @@ def redux_store(store_name, props: {}, defer: false)
@registered_stores ||= []
@registered_stores << redux_store_data
result = render_redux_store_data(redux_store_data)
prepend_render_rails_context(result)
add_render_rails_context(result)
end
end

Expand Down Expand Up @@ -222,31 +226,31 @@ def server_render_js(js_expression, options = {})

private

# prepend the rails_context if not yet applied
def prepend_render_rails_context(render_value)
# add the rails_context if not yet applied
def add_render_rails_context(render_value)
return render_value if @rendered_rails_context

data = {
rails_context: rails_context(server_side: false)
}

@rendered_rails_context = true

rails_context_content = content_tag(:div,
"",
rails_context_content = content_tag(:script,
id: "js-react-on-rails-context",
style: ReactOnRails.configuration.skip_display_none ? nil : "display:none",
data: data)
"#{rails_context_content}\n#{render_value}".html_safe
style: nil) do
"var #{'js-react-on-rails-context'.tr('-', '_')} = #{Yajl.dump(rails_context(server_side: false))};".html_safe
end
"#{render_value}\n#{rails_context_content}".html_safe
end

def render_redux_store_data(redux_store_data)
result = content_tag(:div,
result = content_tag(:script,
"",
class: "js-react-on-rails-store",
style: ReactOnRails.configuration.skip_display_none ? nil : "display:none",
data: redux_store_data)
prepend_render_rails_context(result)
style: nil,
data: redux_store_data) do
# redux_store_data
"var #{redux_store_data[:store_name].tr('-', '_')} = #{Yajl.dump(redux_store_data[:props])};".html_safe
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generating a JavaScript string from Ruby is a pretty bad idea in my experience. For one thing the generated identifier is in the global namespace, but it’s also potentially very fragile.

My personal preference for getting values from Ruby into JS is to provide them as data- attributes on some element, then pull them out with JS… but that seems to be pretty much what you were doing already. Is there some reason why you wanted to use a <script> tag rather than a <div>?

Copy link

@lededje lededje Oct 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global namespace shouldn't be a problem. Attempt to make it as unique as possible though. Prepend it with a bunch of underscores or something similar. I'd advise against using a data attribute though: You might accidentally include an ambiguous ampersand which is not allowed.

Copy link
Contributor

@squadette squadette Nov 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kapowaz a possible reason behind this is that initial props can be quite big, e.g. 100Kb. It is really awkward and unreadable to cram it to HTML attribute, given that every quote is replaced with &quot;.


add_render_rails_context(result)
end

def props_string(props)
Expand Down
1 change: 0 additions & 1 deletion lib/react_on_rails/react_component/options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ def raise_on_prerender_error
def data
{
component_name: name,
props: props,
trace: trace,
dom_id: dom_id
}
Expand Down
4 changes: 2 additions & 2 deletions node_package/src/RenderUtils.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
export default {
wrapInScriptTags(scriptBody) {
wrapInScriptTags(scriptId, scriptBody) {
if (!scriptBody) {
return '';
}

return `\n<script>
return `\n<script id="${scriptId}">
${scriptBody}
</script>`;
},
Expand Down
2 changes: 1 addition & 1 deletion node_package/src/buildConsoleReplay.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,5 @@ export function consoleReplay() {
}

export default function buildConsoleReplay() {
return RenderUtils.wrapInScriptTags(consoleReplay());
return RenderUtils.wrapInScriptTags('consoleReplayLog', consoleReplay());
}
6 changes: 3 additions & 3 deletions node_package/src/clientStartup.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ function turbolinksVersion5() {

function initializeStore(el, railsContext) {
const name = el.getAttribute('data-store-name');
const props = JSON.parse(el.getAttribute('data-props'));
const props = eval(name.replace(/-/g, '_'));
const storeGenerator = ReactOnRails.getStoreGenerator(name);
const store = storeGenerator(props, railsContext);
ReactOnRails.setStore(name, store);
Expand All @@ -55,7 +55,7 @@ function initializeStore(el, railsContext) {
function render(el, railsContext) {
const name = el.getAttribute('data-component-name');
const domNodeId = el.getAttribute('data-dom-id');
const props = JSON.parse(el.getAttribute('data-props'));
const props = eval(domNodeId.replace(/-/g, '_'));
const trace = JSON.parse(el.getAttribute('data-trace'));

try {
Expand Down Expand Up @@ -87,7 +87,7 @@ You should return a React.Component always for the client side entry point.`);
function parseRailsContext() {
const el = document.getElementById('js-react-on-rails-context');
if (el) {
return JSON.parse(el.getAttribute('data-rails-context'));
return eval('js-react-on-rails-context'.replace(/-/g, '_'));
} else {
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion node_package/tests/buildConsoleReplay.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ test('buildConsoleReplay wraps console replay in a script tag', (assert) => {
// https://github.com/jscs-dev/node-jscs/issues/2137
// jscs:disable disallowSpacesInsideTemplateStringPlaceholders
const expected = `
<script>
<script id="consoleReplayLog">
console.log.apply(console, ["some message","{\\"a\\":1,\\"b\\":2}"]);
console.warn.apply(console, ["other message","{\\"c\\":3,\\"d\\":4}"]);
</script>`;
Expand Down
1 change: 1 addition & 0 deletions react_on_rails.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ Gem::Specification.new do |s|
s.add_dependency "rails", ">= 3.2"
s.add_dependency "foreman"
s.add_dependency "addressable"
s.add_dependency "yajl-ruby"

s.add_development_dependency "bundler", "~> 1.10"
s.add_development_dependency "rake", "~> 10.0"
Expand Down
6 changes: 4 additions & 2 deletions spec/dummy/Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ PATH
foreman
rails (>= 3.2)
rainbow (~> 2.1)
yajl-ruby

GEM
remote: https://rubygems.org/
Expand Down Expand Up @@ -98,7 +99,7 @@ GEM
erubis (2.7.0)
execjs (2.7.0)
ffi (1.9.10)
foreman (0.81.0)
foreman (0.82.0)
thor (~> 0.19.1)
generator_spec (0.9.3)
activesupport (>= 3.0.0)
Expand Down Expand Up @@ -278,6 +279,7 @@ GEM
websocket-extensions (0.1.2)
xpath (2.0.0)
nokogiri (~> 1.3)
yajl-ruby (1.2.1)
yard (0.8.7.6)

PLATFORMS
Expand Down Expand Up @@ -319,4 +321,4 @@ DEPENDENCIES
web-console

BUNDLED WITH
1.12.3
1.12.5
68 changes: 33 additions & 35 deletions spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
describe "#react_component" do
before { allow(SecureRandom).to receive(:uuid).and_return(0, 1, 2, 3) }

subject { react_component("App", props: props) }
subject { react_component("App", props: props).squish }

let(:props) do
{ name: "My Test Name" }
Expand All @@ -59,26 +59,24 @@

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

let(:react_definition_div) do
%(<div class="js-react-on-rails-component"
let(:react_definition_script) do
%(<script class="js-react-on-rails-component"
style="display:none"
data-component-name="App"
data-props="{&quot;name&quot;:&quot;My Test Name&quot;}"
data-trace="false"
data-dom-id="#{id}"></div>).squish
data-dom-id="#{id}">var #{id.tr('-', '_')} = {"name":"My Test Name"};</script>).squish
end

let(:react_definition_div_no_params) do
%(<div class="js-react-on-rails-component"
%(<script class="js-react-on-rails-component"
style="display:none"
data-component-name="App"
data-props="{}"
data-trace="false"
data-dom-id="#{id}"></div>).squish
data-dom-id="#{id}">var #{id.tr('-', '_')} = {};</script>).squish
end

describe "API with component name only" do
subject { react_component("App") }
subject { react_component("App").squish }
it { is_expected.to be_an_instance_of ActiveSupport::SafeBuffer }
it { is_expected.to include react_component_div }
it { is_expected.to include react_definition_div_no_params }
Expand All @@ -87,48 +85,46 @@
it { expect(self).to respond_to :react_component }

it { is_expected.to be_an_instance_of ActiveSupport::SafeBuffer }
it { is_expected.to start_with "<div" }
it { is_expected.to match %r{</div>\s*$} }
it { is_expected.to start_with "<script" }
it { is_expected.to match %r{</script>\s*$} }
it { is_expected.to include react_component_div }
it { is_expected.to include react_definition_div }
it { is_expected.to include react_definition_script }

context "with 'id' option" do
subject { react_component("App", props: props, id: id) }

let(:id) { "shaka_div" }
let(:id) { "shaka_script" }

it { is_expected.to include id }
it { is_expected.not_to include react_component_div }
it { is_expected.to include react_definition_div }
it { is_expected.to include react_definition_script }
end

context "with skip_display_none option true" do
before { ReactOnRails.configuration.skip_display_none = true }

let(:react_definition_div_skip_display_none_true) do
"<div class=\"js-react-on-rails-component\"
let(:react_definition_script_skip_display_none_true) do
"<script class=\"js-react-on-rails-component\"
data-component-name=\"App\"
data-props=\"{&quot;name&quot;:&quot;My Test Name&quot;}\"
data-trace=\"false\"
data-dom-id=\"#{id}\"></div>".squish
data-dom-id=\"#{id}\">var #{id.tr('-', '_')} = {\"name\":\"My Test Name\"};</script>".squish
end

it { is_expected.to include react_definition_div_skip_display_none_true }
it { is_expected.to include react_definition_script_skip_display_none_true }
end

context "with skip_display_none option false" do
before { ReactOnRails.configuration.skip_display_none = false }

let(:react_definition_div_skip_display_none_false) do
"<div class=\"js-react-on-rails-component\"
let(:react_definition_script_skip_display_none_false) do
"<script class=\"js-react-on-rails-component\"
style=\"display:none\"
data-component-name=\"App\"
data-props=\"{&quot;name&quot;:&quot;My Test Name&quot;}\"
data-trace=\"false\"
data-dom-id=\"#{id}\"></div>".squish
data-dom-id=\"#{id}\">var #{id.tr('-', '_')} = {\"name\":\"My Test Name\"};</script>".squish
end

it { is_expected.to include react_definition_div_skip_display_none_false }
it { is_expected.to include react_definition_script_skip_display_none_false }
end
end

Expand All @@ -139,35 +135,37 @@
{ name: "My Test Name" }
end

let(:react_store_div) do
%(<div class="js-react-on-rails-store"
let(:react_store_script) do
%(<script class="js-react-on-rails-store"
style="display:none"
data-store-name="reduxStore"
data-props="{&quot;name&quot;:&quot;My Test Name&quot;}"></div>).squish
data-props="{&quot;name&quot;:&quot;My Test Name&quot;}">var reduxStore = {"name":"My Test Name"};
</script>).squish
end

it { expect(self).to respond_to :redux_store }

it { is_expected.to be_an_instance_of ActiveSupport::SafeBuffer }
it { is_expected.to start_with "<div" }
it { is_expected.to end_with "</div>" }
it { is_expected.to include react_store_div }
it { is_expected.to start_with "<script" }
it { is_expected.to end_with "</script>" }
it { is_expected.to include react_store_script }

context "with skip_display_none option true" do
before { ReactOnRails.configuration.skip_display_none = true }

let(:react_store_definition_div_skip_display_none_true) do
%(<div class="js-react-on-rails-store"
let(:react_store_definition_script_skip_display_none_true) do
%(<script class="js-react-on-rails-store"
data-store-name="reduxStore"
data-props="{&quot;name&quot;:&quot;My Test Name&quot;}"></div>).squish
data-props="{&quot;name&quot;:&quot;My Test Name&quot;}">var reduxStore = {"name":"My Test Name"};
</script>).squish
end

it { is_expected.to include react_store_definition_div_skip_display_none_true }
it { is_expected.to include react_store_definition_script_skip_display_none_true }
end

context "with skip_display_none option false" do
before { ReactOnRails.configuration.skip_display_none = false }
it { is_expected.to include react_store_div }
it { is_expected.to include react_store_script }
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/dummy/spec/requests/console_logging_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

expected_lines = expected.split("\n")

script_node = html_nodes.css("script")[1]
script_node = html_nodes.css("#consoleReplayLog")

expected_lines.each do |line|
expect(script_node.inner_text).to include(line)
Expand Down
4 changes: 2 additions & 2 deletions spec/dummy/spec/requests/server_render_check_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,13 @@
mail = DummyMailer.hello_email
expect(mail.subject).to match "mail"
expect(mail.body).to match "Mr. Mailing Server Side Rendering"
expect(mail.body).to match "inMailer&quot;:true"
expect(mail.body).to match "\"inMailer\":true"
end

it "sets inMailer properly" do
get client_side_hello_world_path
html_nodes = Nokogiri::HTML(response.body)
expect(html_nodes.css("div#js-react-on-rails-context").attr("data-rails-context").value)
expect(html_nodes.css("#js-react-on-rails-context").inner_text)
.to match('inMailer\":false')
end
end
Expand Down
1 change: 0 additions & 1 deletion spec/react_on_rails/react_component/options_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ def the_attrs(name: "App", options: {})
attrs = the_attrs(name: "app", options: { trace: false, id: 2 })
expected_data = {
component_name: "App",
props: {},
trace: false,
dom_id: 2
}
Expand Down