-
Notifications
You must be signed in to change notification settings - Fork 754
Inline props operations improvement #208
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
Conversation
…scaped data to the end of body. This makes markup more clean and simple and reduces required bandwith for highload applications.
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks! |
@@ -25,7 +25,7 @@ Gem::Specification.new do |s| | |||
s.add_development_dependency 'jbuilder' | |||
|
|||
s.add_dependency 'execjs' | |||
s.add_dependency 'coffee-script-source', '~>1.9' | |||
s.add_dependency 'coffee-script-source', '~>1.8' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its already been solved in master. Can you sync up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
def add_react_props(props={}) | ||
return if props.empty? | ||
props_id = SecureRandom.base64 | ||
content_for :react_props do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make the props non-cachable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which one element of application stack will lose ability to cache this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
view caches
I think we should make this an opt-in behavior. Add a react config and based on it, allow inline vs script props. |
@vipulnsward Please checkout last CI run. I implemented this with tests but for some versions tests are failing due to issue i wrote about. |
Problem
1.1 Process in browser devtools. So that browser stucks trying to render markup for preview.
1.2 Deliver it to user due to bandwidth (for highload apps).
So props we want to place:
are looking like this:
Solution
Moving inline props to the
<script type="text/json">{"foo": "bar"}</script>
will:Placing inline props at the end of body speeds up DOM rendering and allows Turbolinks to load this data along with other markup on each request.
How it works
The process of placing the components to the markup in Rails is still the same except of developer should add
<%= render_react_props %>
at the end of<body>
tag.P.S.
We selected coffee-script-source at 1.8 because of this issue.