Added callbacks to the Helper to allow you to insert additional JS before and after the server rendering.

[martyphee]

This PR adds pre-render and after-render hooks to allow the insertion of JS before and after server compiling.

Our use case is to allow us to insert i18n js before server rendering so that we have the proper language file.

[justin808]

@martyphee This would not pass the linter. ;-) No double quotes in JS code. So please stick to single quotes. Remember, we're generating JS here, so flip the JS button in your head.

[justin808]

Could you please create the changes for README of how this work?

I think we should make the inline JS code something that one puts in the react_on_rails.rb initializer with the properties

before_server_render_js
after_server_render_js

---

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

---

.gitignore, line 32 [r1] (raw file):
Also, please be sure not to leave trailing spaces.

---

Comments from the review on Reviewable.io

[justin808]

Need to move to using the config file for the the callback of what to generate.

Also, we should pass in information on what is being rendered, so that can be used by some ruby code to dynamically create the before and after JS code.

How about if the config file defines your class name where you put in the methods you wanted to monkey patch?

So you can create a class in the config file and we'll pass in the params that we have (react_component_name, dom_id, etc.) to the constructor of the object, and the creator of this will create a couple methods to return the custom javascript.

Basically, this is just a more elegant plugin than the monkey patching.

---

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

---

README.md, line 454 [r3] (raw file):
we'll use the configuration of the JS in the react_on_rails.js file

No reason to use monkey patching.

---

Comments from the review on Reviewable.io

[justin808]

@martyphee Any updates on this one?

[martyphee]

Sorry, no. It's been a busy week. I'll work on this today. Shouldn't
take long.

On Fri, Feb 26, 2016 at 6:24 PM, Justin Gordon notifications@github.com
wrote:

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

—
Reply to this email directly or view it on GitHub
#287 (comment)
.

[martyphee]

Just pushed an update. I'm having trouble running npm install right now so I haven't been able to test.

[justin808]

@martyphee Any updates?

[justin808]

@martyphee I'm going to be putting in some time this weekend, so let me know if you have any updates.

[martyphee]

I did push updates to this about a week ago.

On Sat, Mar 12, 2016 at 12:45 PM, Justin Gordon notifications@github.com
wrote:

@martyphee https://github.com/martyphee I'm going to be putting in some
time this weekend, so let me know if you have any updates.

—
Reply to this email directly or view it on GitHub
#287 (comment)
.

[justin808]

Hi @martyphee. It's getting there. I left you some comments.

---

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, 2 of 5 files at r5, 4 of 4 files at r6.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

README.md, line 454 [r3] (raw file):
@martyphee We need to remove the monkey patching. The callback class can set in config/react_on_rails.rb and such a callback class should have the two hook methods. I think we should create an instance of the handler object and initialize it with information, such as the component name, props, etc.

---

README.md, line 182 [r6] (raw file):
@martyphee Why is this information in the migrate section?

---

app/helpers/react_on_rails_helper.rb, line 238 [r6] (raw file):
I'm not following this code.

In config/react_on_rails.rb:

config.server_rendering_callback_class = MyServerRenderingCallback

I don't see the need for any sort of dynamic instantiation.

---

lib/react_on_rails/configuration.rb, line 24 [r6] (raw file):
see above comment

we can default to nil if no server_rendering_callback_class

---

lib/react_on_rails/renderer.rb, line 3 [r6] (raw file):
This looks about good.

However, I'm not seeing the need to have a "NULL OBJECT" for this.

---

spec/dummy/Gemfile.lock, line 275 [r6] (raw file):
not really part of this change

---

Comments from the review on Reviewable.io

[justin808]

Please rebase on top of master to a single commit and let me know if you're ready. I'm about to release v4.

[justin808]

@martyphee Back to the need of this...Why are the JS of translations not included in your bundle? Is this to save space in the bundle size?

[justin808]

@martyphee We need a pretty good example of this. The feature is not hard. I just want to make sure the feature is needed.

[martyphee]

Sorry, just got back from London.

We don't include all the translations for each country in the build. Only
the country specific ones. Also we're using Jed instead of the rails i18n
js.

We're not moving anytime soon it seems so unless someone else has a need
for this feel free to close this PR.

On Wed, Mar 16, 2016 at 6:03 PM, Justin Gordon notifications@github.com
wrote:

@martyphee https://github.com/martyphee We need a pretty good example
of this. The feature is not hard. I just want to make sure the feature is
needed.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#287 (comment)

[justin808]

@martyphee This like a potentially useful feature. Possibly there should be different webpack bundles instead? See #343. That way, you could have a helper pick the right server rendering webpack file.

@lucke84 and @jopdeklein, what do you think of this?

[jopdeklein]

@justin808 Focusing on the use case of the locales; we are actually also facing the need to inject just one locale, the Intl and moment locale files are huge so it makes sense to not load them all. However we only need that for the client (we're currently injecting the current locale as inline JS from the template - not ideal but we wanted to avoid the extra external request). For SSR we've just bundled all available locales and pick the correct one runtime based on passed props to the component.

Either way I think this solution is a better approach than to have separate bundles for each locale (eg: bundle-en.js, bundle-nl.js etc) but I wonder how the client will pick up the right locale?

[justin808]

@jopdeklein I'm thinking that we go with @martyphee's feature first. There's a lot more simplicity in creating ONE server side webpack bundle. Different client bundles make a lot of sense.

@alexfedoseev @thewoolleyman @robwise @martyphee @jbhatab Agree?

[martyphee]

Yeah, I'm going to try to get back to this tomorrow and get it merged in.

On Thu, Mar 24, 2016 at 6:44 PM, Justin Gordon notifications@github.com
wrote:

@jopdeklein https://github.com/jopdeklein I'm thinking that we go with
@martyphee https://github.com/martyphee's feature first. There's a lot
more simplicity in creating ONE server side webpack bundle. Different
client bundles make a lot of sense.

@alexfedoseev https://github.com/alexfedoseev @thewoolleyman
https://github.com/thewoolleyman @robwise https://github.com/robwise
@martyphee https://github.com/martyphee @jbhatab
https://github.com/jbhatab Agree?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#287 (comment)

[jopdeklein]

@justin808 I agree, @lucke84 and I will document our rationale for supporting multiple bundles, but in principle you're right, we could likely get away with separated client bundles and a combined bundle for server rendering. I see no issues going ahead with integrating this PR. 👍

[jbhatab]

@jopdeklein @martyphee Just jumping into this one. Trying to identify the main issue that the before and after hooks are addressing. Could you just load in the I18n file in the server rendered file?

[justin808]

If the issue is locales, we should have an example. It would be awesome to have a PR on top of https://github.com/shakacode/react-webpack-rails-tutorial/ showing this. With a solid example, then we have something to go on.

Note, in the next couple of days, I'm going to be changing the JavaScript generator function calls for stores and components so we pass an object containing properties like:

{ url, path, search, hash, {...otherThingsFromRailsRequestLikeLocale} }

Thus, you can configure the locale in your generator function, maybe merging the locale into your props on the client side and only requiring the translation file needed for the given locale.

How's that sound?

[jbhatab]

@justin808 I like that direction more too.

Definitely wanna know if that solves your problems though. I just would like to avoid adding more things like this to the gem if possible.

[martyphee]

This did solve our issue when I did a POC of our react app using
react_on_rails vs react_rails. The latter does support customer pre/post
js render callbacks.

We have to load a JS file per locale before rendering on the server so that
the HTML is in the proper language and then we include the proper locale
file in the head.

On Fri, Mar 25, 2016 at 2:52 PM, Blaine Hatab notifications@github.com
wrote:

@justin808 https://github.com/justin808 I like that direction more too.

Definitely wanna know if that solves your problems though. I just would
like to avoid adding more things like this to the gem if possible.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#287 (comment)

[justin808]

@martyphee

This did solve our issue when I did a POC of our react app using
react_on_rails vs react_rails. The latter does support customer pre/post
js render callbacks.

What is "this"?

If you can point me to a git repo that shows "this" in action, that would be really helpful.

For server rendering with locales, I'm thinking that the server and client code should be the same, and we should be doing an inline "require" of the right locale file based on a value passed into the generator doing the rendering.

I'm guessing that any special code on the hooks you have --> that could also have some logic in the generator function and use a locale value passed in.

[martyphee]

"this"? deliveroo.co.uk. It's currently angular but we're rewriting it in
React. The current implementation is using react_rails, but we need to
move to webpack build so I did a POC with react_on_rails which had a
custom renderer that I modeled off what react_rails did.

I can't point you to the repo since it's private.

I'm thinking that the server and client code should be the same
They are more or less. In the custom rendered I look at the I18n.locale
and pull in the correct translations for the country.

i18n = ::Rails.application.assets["locale/macaroon-#{I18n.locale}.js"].to_s

On Fri, Mar 25, 2016 at 3:45 PM, Justin Gordon notifications@github.com
wrote:

@martyphee https://github.com/martyphee

This did solve our issue when I did a POC of our react app using
react_on_rails vs react_rails. The latter does support customer pre/post
js render callbacks.

What is "this"?

If you can point me to a git repo that shows "this" in action, that would
be really helpful.

For server rendering with locales, I'm thinking that the server and client
code should be the same, and we should be doing an inline "require" of the
right locale file based on a value passed into the generator doing the
rendering.

I'm guessing that any special code on the hooks you have --> that could
also have some logic in the generator function and use a locale value
passed in.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#287 (comment)

[jbhatab]

@martyphee could you put the js you need in a layout or view file before the component all since that will be server rendered?

[martyphee]

We need to have the i18n available during server rendering so that it has
the proper language. In our jsx we have code like this:

placeholder={ i18n.gettext('e.g. %(postcode)s', { postcode:
this.props.examplePostcode }) }

i18n is defined like so in an included file. We using FastGetText for
our translations on the server and Jed on the client. The json below is
built with po2json.

//= require jed/jed
//= require locale/i18n

//= require_self

jed = new Jed({
   "domain": "orderwebjsx",
   "locale_data": {
      "orderwebjsx": {
         "": {
            "domain": "orderwebjsx",
            "plural_forms": "nplurals=2; plural=(n != 1);",
            "lang": "de"
         },
         "%(name)s": [
            "%(name)s"
         ],
         "Choose your language": [
            "Wähle deine Sprache"
         ],
         "e.g. %(postcode)s": [
            "z.B. %(postcode)s"
         ],

On Fri, Mar 25, 2016 at 4:12 PM, Blaine Hatab notifications@github.com
wrote:

@martyphee https://github.com/martyphee could you put the js you need
in a layout or view file before the component all since that will be server
rendered?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#287 (comment)

[justin808]

Let's move this discussion over to #345.

I think this will do everything this was going to do and a whole lot more.

[justin808]

@martyphee Please see #345. I plan to merge later today. I think this should meet your needs.

[justin808]

@martyphee I'd like to see if we can handle your case for the 5.0.0 final release.

[justin808]

@earaujoassis, please discuss with @martyphee if we can solve the localization issue without the extra hooks. I'm very happy to put in the hooks, but I need a shareable example that justifies this.

[justin808]

@martyphee Should we close this?

[martyphee]

Yes.

On Wednesday, April 20, 2016, Justin Gordon notifications@github.com
wrote:

@martyphee https://github.com/martyphee Should we close this?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#287 (comment)

[stuarthannig]

I think the hooks + railsContext are great additions. Hooks could solve issues around server rendering that client rendering doesn't need to concern itself about, and thus doesn't need to be in railsContext. For example you may need to construct dependancies (eg, a psuedo-window object) so server rendering doesn't error, etc.

+1 for hooks

[justin808]

@stuarthannig Feel free to create a new PR with an example that justifies this feature.