Change div tags to script tags for props #411

martyphee · 2016-05-04T12:57:35Z

WIP - Changed div's to script tags.

This change is

… nodes or the redux store names. 3 tests failing yet.

justin808 · 2016-06-26T00:16:42Z

@martyphee Any update on this one?

martyphee · 2016-07-01T06:55:03Z

Sorry, no. I'll be working on it again this week. I've been in London
with my family for two weeks and haven't had time to work on it.

On Sun, Jun 26, 2016 at 1:16 AM, Justin Gordon [email protected]
wrote:

@martyphee https://github.com/martyphee Any update on this one?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#411 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAuEtvZVyx7pTWDjrYKIeRWSQVzPPEnjks5qPcTsgaJpZM4IXHeO
.

jbhatab · 2016-07-08T00:03:50Z

@martyphee let me know if you wanna chat about this at all too.

martyphee · 2016-07-11T21:28:31Z

Yes, do you have any time tomorrow?

On Thu, Jul 7, 2016 at 7:03 PM, Blaine Hatab [email protected]
wrote:

@martyphee https://github.com/martyphee let me know if you wanna chat
about this at all too.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#411 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAuEtlMThyeMifROgTFfnEuxseackYSHks5qTZPogaJpZM4IXHeO
.

justin808 · 2016-07-11T23:30:56Z

Please refer to:

To merge, we need:

Ensure this works just fine with both server and client rendering.
Ensure this works flawlessly with Rails 5 and Turbolinks 5.
Ensure that we've properly addressed all possible sources of injected scripts.
Ensure that the performance gain is worth any extra complexity.

Take a look at the home page for https://www.producthunt.com/

Seems that ProductHunt uses this strategy of a script.

They probably have done tons of optimization.

Interesting that the embedded store values seem to be a string, rather than an object.

    window.initialState = "[\"^ \",\"annotations\",[\"~#iM\",[69195,[\"~#iR\",[\"^ \",\"n\",\"AnnotationRecord\",\"v\",[\"^ \",\"type\",\"top\",\"postId\",null,\"categoryId\",1,\"userCount\",0,\"userIds\",[]]]],69203,[\"^2\",[\"^ \",\"n\",\"AnnotationRecord\",\"v\",[\"^ \",\"^3\",\"top

So maybe the strategy is to convert to an HTML JSON string, and then JSON parse the string?

justin808 · 2016-07-22T03:48:18Z

@martyphee Any update?

justin808 · 2016-08-01T01:09:58Z

@martyphee How's this one going? Any preliminary performance numbers?

martyphee · 2016-08-01T19:55:02Z

No, sorry. I haven't had time to work on it. I'll try to spend time this week before I go on vacation. Just got back from UK.

justin808 · 2016-08-05T06:14:46Z

@martyphee Keep us posted. Hopefully you'll have some perf numbers to share!

justin808 · 2016-08-08T09:45:15Z

@martyphee How's this one going?

martyphee · 2016-08-11T21:20:57Z

Sorry for the delay. I'm on vacation right now. No plans to head over seas
for a while so I hope to revisit not week.

On Monday, August 8, 2016, Justin Gordon [email protected] wrote:

@martyphee https://github.com/martyphee How's this one going?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#411 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuEtmsYvn9w6ngmlid86AqdnAENTWjfks5qdvqugaJpZM4IXHeO
.

justin808 · 2016-08-20T06:59:28Z

Hi @martyphee I'm about to push out 6.1 this weekend. Would you like to try to get this in?

…re those out.

martyphee · 2016-08-20T13:22:41Z

I have two failing tests yet.

1) 2 react components, 1 store, client only, controller setup /client_side_hello_world_shared_store_controller Type in one component changes the other component
     Failure/Error: background { visit url }

     Capybara::Poltergeist::DeadClient:
       PhantomJS client died while processing {"id":"3d24856d-407d-4175-8646-9fc1db792bff","name":"visit","args":["http://127.0.0.1:63041/client_side_hello_world_shared_store_controller",30]}
     Shared Example Group: "React Component Shared Store" called from ./spec/features/integration_spec.rb:184
     # ./spec/features/integration_spec.rb:148:in `block (2 levels) in <top (required)>'

  2) 2 react components, 1 store, server side, defer /server_side_hello_world_shared_store_defer Type in one component changes the other component
     Failure/Error: background { visit url }

     Capybara::Poltergeist::DeadClient:
       PhantomJS client died while processing {"id":"f94da957-ffcd-4022-853a-7d0d2c61caa0","name":"visit","args":["http://127.0.0.1:63041/server_side_hello_world_shared_store_defer",30]}
     Shared Example Group: "React Component Shared Store" called from ./spec/features/integration_spec.rb:196
     # ./spec/features/integration_spec.rb:148:in `block (2 levels) in <top (required)>'

Need to figure these out.

# Conflicts: # spec/dummy/Gemfile.lock

justin808 · 2016-08-20T19:57:52Z

@martyphee Please address these in the PR description and the final commit. #411 (comment)

justin808 · 2016-08-21T03:13:46Z

@martyphee Here's a security reference on the XSS topic: https://rorsecurity.info/cross-site-scripting-xss-rails

justin808 · 2016-09-04T07:26:01Z

@martyphee Any new info on this one?

justin808 · 2016-09-26T05:21:29Z

@martyphee How's this one going?

justin808 · 2016-10-06T04:25:45Z

@martyphee Any more details on this one?

justin808 · 2016-10-22T05:09:10Z

@martyphee Are you still interested in this fix?

kapowaz · 2016-10-25T09:18:01Z

app/helpers/react_on_rails_helper.rb

+                         data: redux_store_data) do
+      # redux_store_data
+      "var #{redux_store_data[:store_name].tr('-', '_')} = #{Yajl.dump(redux_store_data[:props])};".html_safe
+    end


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>?

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.

@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 ".

martyphee · 2016-10-25T11:55:23Z

@justin808 Yes, we still want to do this to try speeding up the page rendering. I'll work on it this week.

justin808 · 2016-11-01T05:26:41Z

@martyphee How's this one going?

martyphee · 2016-11-01T10:22:58Z

I started to do some work on it last night. I'll try to get thru it
tonight. In Edinburgh right now.

On Tue, Nov 1, 2016 at 5:26 AM, Justin Gordon [email protected]
wrote:

@martyphee https://github.com/martyphee How's this one going?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#411 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuEtlWQ1lWUoRiPB2n_jpSelo3RI_B-ks5q5s2TgaJpZM4IXHeO
.

squadette · 2016-11-06T22:48:58Z

app/helpers/react_on_rails_helper.rb

+                  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


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

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()!

martyphee · 2016-11-10T14:53:33Z

I haven't had much time to look at this yet.

justin808 · 2016-12-01T00:28:23Z

Hi @martyphee Any new developments on this one?

justin808 · 2016-12-22T08:04:04Z

@martyphee Is this one worth doing? @squadette any interest in working on this one?

justin808 · 2017-01-02T02:09:12Z

See #660.

justin808 · 2017-02-02T04:39:49Z

Deferring to #660.

martyphee added 2 commits May 4, 2016 07:56

Changed the tags to script tags instead of div's for the data.

5ca16db

Moved that data into the script tag.

d452721

justin808 added the enhancement label May 6, 2016

martyphee added 4 commits May 21, 2016 12:02

Merge branch 'master' into change-div-tags-to-script-tags-for-props

f9331b6

Moved that data into the script tag.

6ca0030

Merge branch 'master' into change-div-tags-to-script-tags-for-props

aab243c

Updates to make the json into variables which are named after the dom…

5ad21af

… nodes or the redux store names. 3 tests failing yet.

justin808 added the waiting for submitter label Jul 22, 2016

justin808 modified the milestone: 6.1 Aug 8, 2016

Fixing some final test. There are two which are failing. Need to figu…

ff0c968

…re those out.

martyphee added 5 commits August 20, 2016 08:29

Merge branch 'master' into change-div-tags-to-script-tags-for-props

edd0ed0

# Conflicts: # spec/dummy/Gemfile.lock

Fixing a couple more tests.

7c892d7

Fixing one more test. Can't run locally for some reason.

0663cee

Made a mistake in the test

f81fb59

Fixing lint errors.

64a7ced

martyphee added 2 commits August 20, 2016 14:42

Fixing lint errors.

bebf370

Fixing lint errors.

afeea39

kapowaz reviewed Oct 25, 2016

View reviewed changes

squadette reviewed Nov 6, 2016

View reviewed changes

justin808 mentioned this pull request Nov 22, 2016

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

Closed

justin808 closed this Feb 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change div tags to script tags for props #411

Change div tags to script tags for props #411

martyphee commented May 4, 2016 •

edited by justin808

Loading

justin808 commented Jun 26, 2016

martyphee commented Jul 1, 2016

jbhatab commented Jul 8, 2016

martyphee commented Jul 11, 2016

justin808 commented Jul 11, 2016

justin808 commented Jul 22, 2016

justin808 commented Aug 1, 2016

martyphee commented Aug 1, 2016 via email

justin808 commented Aug 5, 2016

justin808 commented Aug 8, 2016

martyphee commented Aug 11, 2016

justin808 commented Aug 20, 2016

martyphee commented Aug 20, 2016 •

edited

Loading

justin808 commented Aug 20, 2016

justin808 commented Aug 21, 2016

justin808 commented Sep 4, 2016

justin808 commented Sep 26, 2016

justin808 commented Oct 6, 2016

justin808 commented Oct 22, 2016

kapowaz Oct 25, 2016

lededje Oct 25, 2016 •

edited

Loading

squadette Nov 6, 2016 •

edited

Loading

martyphee commented Oct 25, 2016

justin808 commented Nov 1, 2016

martyphee commented Nov 1, 2016

squadette Nov 6, 2016

squadette Nov 6, 2016 •

edited

Loading

martyphee commented Nov 10, 2016

justin808 commented Dec 1, 2016

justin808 commented Dec 22, 2016

justin808 commented Jan 2, 2017

justin808 commented Feb 2, 2017

Change div tags to script tags for props #411

Change div tags to script tags for props #411

Conversation

martyphee commented May 4, 2016 • edited by justin808 Loading

justin808 commented Jun 26, 2016

martyphee commented Jul 1, 2016

jbhatab commented Jul 8, 2016

martyphee commented Jul 11, 2016

justin808 commented Jul 11, 2016

justin808 commented Jul 22, 2016

justin808 commented Aug 1, 2016

martyphee commented Aug 1, 2016 via email

justin808 commented Aug 5, 2016

justin808 commented Aug 8, 2016

martyphee commented Aug 11, 2016

justin808 commented Aug 20, 2016

martyphee commented Aug 20, 2016 • edited Loading

justin808 commented Aug 20, 2016

justin808 commented Aug 21, 2016

justin808 commented Sep 4, 2016

justin808 commented Sep 26, 2016

justin808 commented Oct 6, 2016

justin808 commented Oct 22, 2016

kapowaz Oct 25, 2016

Choose a reason for hiding this comment

lededje Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

squadette Nov 6, 2016 • edited Loading

Choose a reason for hiding this comment

martyphee commented Oct 25, 2016

justin808 commented Nov 1, 2016

martyphee commented Nov 1, 2016

squadette Nov 6, 2016

Choose a reason for hiding this comment

squadette Nov 6, 2016 • edited Loading

Choose a reason for hiding this comment

martyphee commented Nov 10, 2016

justin808 commented Dec 1, 2016

justin808 commented Dec 22, 2016

justin808 commented Jan 2, 2017

justin808 commented Feb 2, 2017

martyphee commented May 4, 2016 •

edited by justin808

Loading

martyphee commented Aug 20, 2016 •

edited

Loading

lededje Oct 25, 2016 •

edited

Loading

squadette Nov 6, 2016 •

edited

Loading

squadette Nov 6, 2016 •

edited

Loading