Feature/node

[alexeuler]

NodeJS as a backend for server-side rendering.

To switch from NodeJS to ExecJS and vice-versa

1. Open config/initializers/react_on_rails.rb in your project
2. Change server_render_method param to either NodeJS or ExecJS

To make a simple load test, after starting the server use rake load_test:run in your project folder.
Args:

• url - url to be tested, default - http://localhost:3000/hello_world
• count - number of requests, default - 500

Before running load test, make sure that Apache Benchmark is installed on your system (installed on OSX by default)

---

This change is

[justin808]

Thanks @alleycat-at-git. I put in some questions.

---

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

---

lib/generators/react_on_rails/templates/base/base/Procfile.dev.tt, line 5 [r1] (raw file):
I'm curious about the scalability when using heroku. If this is very slightly faster, but you are running two processes, then the comparison is two processes for the ruby server, not one.

---

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 45 [r1] (raw file):
Nice to have this option.

---

lib/generators/react_on_rails/templates/base/base/lib/tasks/load_test.rake, line 8 [r1] (raw file):
something wrong with the end of file. We want one return.

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 2 [r1] (raw file):
we can remove the header part - contributors are listed in the history and the changelog.

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 9 [r1] (raw file):
what's this comment for

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 19 [r1] (raw file):
will this catch any time the server-bundle.js starts? Also, this is a configuration parameter, so at the minimum, we need to comment that if you change the name of the bundle, then you need to change this file.

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 27 [r1] (raw file):
all 'var' should be const or let, right?

Are we parsing the entire server-bundle.js every time?

---

lib/react_on_rails/server_rendering_pool.rb, line 21 [r1] (raw file):
why method missing?

I think an if statement is probably fine. Even if we want ultimate customization, we can have an else condition.

---

lib/react_on_rails/server_rendering_pool/exec.rb, line 3 [r1] (raw file):
We should make it clear that this is using execjs

---

lib/react_on_rails/server_rendering_pool/exec.rb, line 148 [r1] (raw file):
how does this compare to the node way? Do we get the native console output?

---

lib/react_on_rails/server_rendering_pool/node.rb, line 9 [r1] (raw file):
timeout? pool size?

---

lib/react_on_rails/server_rendering_pool/node.rb, line 10 [r1] (raw file):
seems like this applies to execjs

---

lib/react_on_rails/server_rendering_pool/node.rb, line 25 [r1] (raw file):
can this be combined with the execjs code?

---

lib/react_on_rails/server_rendering_pool/node.rb, line 37 [r1] (raw file):
why does the range go 2..-2?

---

lib/react_on_rails/server_rendering_pool/node.rb, line 52 [r1] (raw file):
this probably applies to execjs as well

---

lib/react_on_rails/server_rendering_pool/node.rb, line 69 [r1] (raw file):
How do we know what the max response should be?

How many node instances should the node server be running? Does this require buying extra instances from Heroku?

---

lib/react_on_rails/server_rendering_pool/node.rb, line 87 [r1] (raw file):
be sure to run rake lint

---

Comments from Reviewable

[jbhatab]

lib/generators/react_on_rails/templates/base/base/Procfile.dev.tt, line 5 [r1] (raw file):
@justin808 @alleycat-at-git I think this will require getting an extra node for the node server if pushed to heroku but this is only for development.

---

Comments from Reviewable

[jbhatab]

lib/generators/react_on_rails/templates/base/base/lib/tasks/load_test.rake, line 8 [r1] (raw file):
I don't think we should add this to the core of the gem. We can put this in a readme doc though which i think would be great. I love the concept.

---

Comments from Reviewable

[jbhatab]

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 2 [r1] (raw file):
yeah, agreed. no need for extra comments. needed.

---

Comments from Reviewable

[jbhatab]

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 19 [r1] (raw file):
agreed on the variable for the filename. @justin808 can we pass that option in from rails because that would be ideal. Maybe in the node process for the Pocfile.dev.

---

Comments from Reviewable

[justin808]

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

---

lib/generators/react_on_rails/templates/base/base/lib/tasks/load_test.rake, line 8 [r1] (raw file):
We'd have a separate folder for performance tests. It's definitely something we want in the project.

---

Comments from Reviewable

[jbhatab]

lib/react_on_rails/server_rendering_pool/exec.rb, line 3 [r1] (raw file):
Is the title not clear enough?

---

Comments from Reviewable

[jbhatab]

lib/react_on_rails/configuration.rb, line 60 [r1] (raw file):
Can we just make this a boolean that says node_server and it defaults to false?

---

Comments from Reviewable

[jbhatab]

lib/react_on_rails/server_rendering_pool/node.rb, line 25 [r1] (raw file):
@alleycat-at-git @justin808 I may be off on this, but why do we even need this file? Won't node be doing all of the work and outputting all of the console code?

---

Comments from Reviewable

[jbhatab]

lib/react_on_rails/server_rendering_pool/node.rb, line 69 [r1] (raw file):
Yeah I would want to know if it requires an extra instance as well, but we no longer have heroku in the gem so at least it won't be enforcing this for production. We will want to update the readme for heroku to reflect this though. The gem may not support it but the docs should.

---

Comments from Reviewable

[jbhatab]

lib/generators/react_on_rails/templates/base/base/lib/tasks/load_test.rake, line 8 [r1] (raw file):
I don't think it's something that belongs in the core of the gem. At that point then every persons preference on load testing should be implemented. I think these are best suited for how-tos or readmes.

---

Comments from Reviewable

[jbhatab]

@alleycat-at-git So this is a interesting PR, but at a high level do we really want this in the core of the gem? Just curious what your reasoning is.

---

Comments from Reviewable

[alexeuler]

Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

---

lib/generators/react_on_rails/templates/base/base/Procfile.dev.tt, line 5 [r1] (raw file):
I might be not in the mainstream, but I always prefer VPS over Heroku. In terms of blank hello world app, the node process consumes, 18mb of memory and rails process - 32 mb. That means that you can launch 2 node processes per one rails process. Furthermore as the app grows - rails app grows pretty fast in terms of memory consumption, e.g. 100mb is pretty common. I'm sure, the node process will grow way much slower, so expect the ratio to be 4 to 1 when you ship your app.

The other thing is that node uses event-based async model, which is hugely more effective than rails default sync calls. So, for instance, if the JS will communicate with a 3rd party web-service, the effectiveness of one node process will be a magnitude higher than that of the rails process.

As for heroku, AFAIK, you really have to spend the same money for rails and node process.

---

lib/generators/react_on_rails/templates/base/base/lib/tasks/load_test.rake, line 8 [r1] (raw file):
Will add the end of line.

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 2 [r1] (raw file):
The default feature for Rubymine, I will clean this up

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 9 [r1] (raw file):
This one too

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 19 [r1] (raw file):
Agreed, will do that

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 27 [r1] (raw file):

all 'var' should be const or let, right?
As of ES6 - yes! Node however still operates under ES5.
Are we parsing the entire server-bundle.js every time?
Nope, we just execute the incoming JS in the context

---

lib/react_on_rails/configuration.rb, line 60 [r1] (raw file):
We could, but this will limit ourselves when we might come up with the 3rd solution for the rendering.

---

lib/react_on_rails/server_rendering_pool.rb, line 21 [r1] (raw file):
It's just a simple way for the has-a inheritance. I don't see anything bad with that. Or am I missing something?

---

lib/react_on_rails/server_rendering_pool/exec.rb, line 3 [r1] (raw file):
Will add a comment

---

lib/react_on_rails/server_rendering_pool/node.rb, line 9 [r1] (raw file):
Yep, this is the connection pool with timeout if we can't use the connection.

---

lib/react_on_rails/server_rendering_pool/node.rb, line 10 [r1] (raw file):
Probably not the best naming, but I think we better leave it just to be similar to ExecJs implementation.

---

lib/react_on_rails/server_rendering_pool/node.rb, line 25 [r1] (raw file):

can this be combined with the execjs code?
Yep, we definitely can resuse some code, but I think that is the matter of the next iteration.
Won't node be doing all of the work and outputting all of the console code?
As far as I understood, this file logs the exact JS code received to render, node won't do that by default.

---

lib/react_on_rails/server_rendering_pool/node.rb, line 37 [r1] (raw file):
I have no idea, just copypasted from the execJS implementation )) Obviously it cuts some output from the beginning and the end. Probably the output is not important.

---

lib/react_on_rails/server_rendering_pool/node.rb, line 69 [r1] (raw file):

How do we know what the max response should be?
That's fair, it's just a quick and dirty way, will refactor this into more robust version

How many node instances should the node server be running?
One will do just fine

For Heroku see my comment above

---

lib/react_on_rails/server_rendering_pool/node.rb, line 87 [r1] (raw file):
Ок

---

Comments from Reviewable

[alexeuler]

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 19 [r1] (raw file):
Moved this parameter into the npm start script at client/node/package.json

---

Comments from Reviewable

[alexeuler]

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 19 [r1] (raw file):
Done.

---

Comments from Reviewable

[alexeuler]

Review status: 3 of 10 files reviewed at latest revision, 18 unresolved discussions.

---

lib/react_on_rails/server_rendering_pool/exec.rb, line 3 [r1] (raw file):
Done.

---

lib/react_on_rails/server_rendering_pool/exec.rb, line 148 [r1] (raw file):
Yes we do

---

lib/react_on_rails/server_rendering_pool/node.rb, line 87 [r1] (raw file):
Done.

---

Comments from Reviewable

[alex35mil]

My guess why "only 5% performance improvement": node shines not in the synchronous code evaluation, but in asynchronous requests handling, so it can efficiently deal w/ multiple incoming requests at a time. But since those incoming requests are still handled by ruby server, node does only js execution job. The huge advantage of this PR is that now we have event loop and all others modern JS features, which is a BIG deal.

---

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

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 20 [r2] (raw file):
This one is deprecated.

---

Comments from Reviewable

[jbhatab]

lib/generators/react_on_rails/templates/base/base/Procfile.dev.tt, line 5 [r1] (raw file):
Yeah a good VPS is fantastic. So that's interesting that it brings that to the table. It doesn't seem like it will change too much code either. It sounds like you have more than enough use cases too.

---

Comments from Reviewable

[jbhatab]

lib/react_on_rails/configuration.rb, line 60 [r1] (raw file):
Yeah might as well leave it open then.

---

Comments from Reviewable

[jbhatab]

@alexfedoseev Thanks for clearing that up. So this would enable something like an alliterative for rails web sockets for a chat bundle?

Also will this work perfectly fine in production? I was still a little confused on that. I thought the app built all the js out to a single file for production, so does that mean node can basically take a completed webpack file and run it on a server?

---

Comments from Reviewable

[justin808]

This looks pretty good. Once we have the test suite for capybara configured to do a run with this, we can probably release this as either "working" or "experimental".

@alexfedoseev thoughts?

---

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

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 23 [r1] (raw file):
did you want to keep this comment? @alleycat-at-git

---

lib/react_on_rails/server_rendering_pool.rb, line 21 [r1] (raw file):
Can you please give an example of how this is used? There could be a performance issue with the dynamic call (if that's the normal case).

---

lib/react_on_rails/server_rendering_pool/node.rb, line 9 [r1] (raw file):
should this be a different variable then?

---

lib/react_on_rails/server_rendering_pool/node.rb, line 37 [r1] (raw file):
this should be examined with the node version.

---

lib/react_on_rails/server_rendering_pool/node.rb, line 4 [r2] (raw file):
execjs?

---

Comments from Reviewable

[alexeuler]

Review status: 8 of 10 files reviewed at latest revision, 10 unresolved discussions.

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 23 [r1] (raw file):
Done.

---

lib/generators/react_on_rails/templates/base/server_rendering/client/node/server.js, line 20 [r2] (raw file):
Done.

---

lib/react_on_rails/server_rendering_pool.rb, line 21 [r1] (raw file):
For instance when you call ServerRenderingPool.reset_pool it just forwards it to either ServerRendringPool::Exec.reset_pool or to ServerRendringPool::Node.reset_pool depending on the config. I don't think there'll be any performance issues with this. The hierarchy for ServerRenderingPool is really shallow so it will almost immediately go to method missing and forward the message

---

lib/react_on_rails/server_rendering_pool/node.rb, line 9 [r1] (raw file):
I incline to leave it the same, although there are pros and cons. Technically we could call it server_rendering_connection_pool_size, but doing so we might confuse the end user as there'll be 2 types of variables for node and exec. I think we could avoid that - after all in both cases we have pools and the bigger the pool the more effective we render.

---

lib/react_on_rails/server_rendering_pool/node.rb, line 37 [r1] (raw file):
I examined this thing and it is used solely to log the output of the exec js to console. Since console in node can do this automatically - no need for this whole block.

---

lib/react_on_rails/server_rendering_pool/node.rb, line 52 [r1] (raw file):
I have no idea about who might use this one. It just logs the js into a file, I can delete it, but the author might get offended )

---

lib/react_on_rails/server_rendering_pool/node.rb, line 4 [r2] (raw file):
Done.

---

Comments from Reviewable

[justin808]

Let's try to get this in by next Monday. We want to:

1. Squash commits to single one
2. Have decent docs
3. Sync to master
4. Make sure CI passes

[coveralls]

Coverage decreased (-2.9%) to 82.452% when pulling 7c3c314 on alleycat-at-git:feature/node into bf834bc on shakacode:master.

[justin808]

@alleycat-at-git Are we ready to merge this? Maybe squash the commits to one and rebase on top of master and I'll merge. The key is that this doesn't go on by default and that it's labelled as experimental.

[coveralls]

Coverage decreased (-2.9%) to 82.452% when pulling 2d674c9 on alleycat-at-git:feature/node into 9995619 on shakacode:master.

[coveralls]

Coverage decreased (-2.9%) to 82.452% when pulling 95391af on alleycat-at-git:feature/node into 9995619 on shakacode:master.

[coveralls]

Coverage decreased (-2.9%) to 82.452% when pulling 95391af on alleycat-at-git:feature/node into 9995619 on shakacode:master.

[alexeuler]

@justin808 Ready to merge!

---

Review status: 6 of 15 files reviewed at latest revision, 8 unresolved discussions.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-2.9%) to 82.452% when pulling a3fac01 on alleycat-at-git:feature/node into 9995619 on shakacode:master.

[jbhatab]

@justin808 is this good to go?

[justin808]

🔥

@alleycat-at-git Please add a changelog entry and then I'll merge!

@jbhatab, @robwise from now on, all PRs (except for doc) need a changelog entry in the unreleased section:

https://github.com/shakacode/react_on_rails/blob/master/CHANGELOG.md#unreleased

---

Reviewed 2 of 3 files at r4, 5 of 6 files at r5, 1 of 1 files at r6, 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

lib/react_on_rails/server_rendering_pool/node.rb, line 52 [r1] (raw file):
@alleycat-at-git Very useful for debugging! To see the code we sent for server rendering.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-2.9%) to 82.452% when pulling 9951de4 on alleycat-at-git:feature/node into e035e54 on shakacode:master.

[coveralls]

Coverage decreased (-2.9%) to 82.452% when pulling 9951de4 on alleycat-at-git:feature/node into e035e54 on shakacode:master.

[alexeuler]

@justin808 Done! Regarding tests, we need to run the whole test suite against both rendering options in the CI. It might be premature in the experimental stage, but needs to be implemented when we have full-blown feature. Right now the feature pass all the test and must be fully covered, so no problems with that.

---

Review status: 15 of 16 files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-2.9%) to 82.452% when pulling 9951de4 on alleycat-at-git:feature/node into e035e54 on shakacode:master.

[coveralls]

Coverage decreased (-2.9%) to 82.452% when pulling 9951de4 on alleycat-at-git:feature/node into e035e54 on shakacode:master.

[justin808]

---

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

---

Comments from Reviewable

[justin808]

@alleycat-at-git Please rebase on top of master, and I'll merge.

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[alexeuler]

@justin808 Done!

---

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-2.9%) to 82.452% when pulling e48f6bc on alleycat-at-git:feature/node into 35e1861 on shakacode:master.

[justin808]

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable